histogram メトリクスの bucket を設定できるようにする #260
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brly
previously approved these changes
Dec 10, 2019
shinnya
reviewed
Dec 14, 2019
scripts/setup_debug_cluster.sh
Outdated
| @@ -19,6 +19,8 @@ TOLERABLE_FAULTS=${TOLERABLE_FAULTS:-$(( ($SERVER_COUNT / 2 - 1) >= 1 ? ($SERVER | |||
| # Note: $SERVER_COUNT + 1 は contact-server の分を $SERVER_COUNT に追加している。 | |||
| DATA_FRAGMENTS=${DATA_FRAGMENTS:-$(( ($SERVER_COUNT + 1 - $TOLERABLE_FAULTS) >= 1 ? ($SERVER_COUNT + 1 - $TOLERABLE_FAULTS) : 1 ))} | |||
| FRUGALOS_START_FLAGS=${FRUGALOS_START_FLAGS:- --sampling-rate 1.0} | |||
| FRUGALOS_CONFIG_FILE=${PWD}/docker/frugalos/frugalos.yml | |||
There was a problem hiding this comment.
docker/frugalos/frugalos.yml は docker 用の設定ファイルであって、このような依存関係を作ると変更の影響範囲が読めなくなるので、 scripts/ に設定ファイルを別途置いた方が良いんじゃないでしょうか。
shinnya
reviewed
Dec 14, 2019
src/lib.rs
Outdated
| @@ -157,13 +158,17 @@ pub struct FrugalosDaemonConfig { | |||
| /// Jaegerのトレースのサンプリング確率。 | |||
| #[serde(default = "default_sampling_rate")] | |||
| pub sampling_rate: f64, | |||
| /// daemon 管理の HTTP サーバのメトリクスのバケツの設定 | |||
| #[serde(default)] | |||
| pub bucket_config: HistogramBucketConfig, | |||
There was a problem hiding this comment.
以下の点で設定が分かりづらいように感じます。
- 名前からすると一見 frugalos のバケツに対する設定と混同しやすい
- 何に対して適用されるのかパッと見て不明。
http_serverの設定とこの設定は何が違うのか？なぜ設定が別れているのか？が直感的には理解できない。
There was a problem hiding this comment.
- について、daemon の設定でも
.frugalos.http_server以下に書いた方がいいですかね?
1., 2. は直します。
shinnya
reviewed
Dec 14, 2019
src/lib.rs
Outdated
| @@ -174,12 +179,16 @@ pub struct FrugalosHttpServerConfig { | |||
| /// bind するアドレス。 | |||
| #[serde(default = "default_http_server_bind_addr")] | |||
| pub bind_addr: SocketAddr, | |||
| /// HTTP サーバのメトリクスのバケツの設定 | |||
| #[serde(default)] | |||
| pub bucket_config: HistogramBucketConfig, | |||
There was a problem hiding this comment.
どのメトリクスに適用される設定なのかをどこかに明示した方が親切ではないでしょうか。個人的な好みでは、メトリクス名を設定名内で明示するのが分かりやすくてよいかなと感じます。
017c70c to
1ece4e4
Compare
shinnya
reviewed
Jan 7, 2020
shinnya
reviewed
Jan 7, 2020
| @@ -19,6 +19,8 @@ TOLERABLE_FAULTS=${TOLERABLE_FAULTS:-$(( ($SERVER_COUNT / 2 - 1) >= 1 ? ($SERVER | |||
| # Note: $SERVER_COUNT + 1 は contact-server の分を $SERVER_COUNT に追加している。 | |||
| DATA_FRAGMENTS=${DATA_FRAGMENTS:-$(( ($SERVER_COUNT + 1 - $TOLERABLE_FAULTS) >= 1 ? ($SERVER_COUNT + 1 - $TOLERABLE_FAULTS) : 1 ))} | |||
| FRUGALOS_START_FLAGS=${FRUGALOS_START_FLAGS:- --sampling-rate 1.0} | |||
| FRUGALOS_CONFIG_FILE=`cd \`dirname $0\`; pwd`/frugalos.yml | |||
There was a problem hiding this comment.
直した方がいいという話ではないです。
些細な話ですが back quote ではなく $() を使えばエスケープする必要がないです。
shinnya
previously approved these changes
Jan 7, 2020
|
|
e2bd444 to
fdfccd6
Compare
shinnya
approved these changes
Jan 9, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Types of changes
Please check one of the following:
masteranddevelop!)Description of changes
Behavior
frugalos の設定ファイルの
frugalos.fibers_http_server.request_duration_bucket_configが、HTTP レスポンス時間の histogram のバケツ設定を受け取れるようになる。(sile/fibers_http_server#3 の変更を取り込むため。)また、
scripts/setup_debug_cluster.shがscripts/frugalos.ymlを設定ファイルとして frugalos を起動するようになる。また、 fibers_http_server が 0.1.8 から 0.1.12 にバージョンアップする (diff) ので、以下の副作用が入る:
fibers_http_server::server::Serverの Drop 時に接続を切るPurpose
この実装を入れていない状態だと、バケツが
0.1, 0.5, 1.0, 5.0, ...という設定になっており、例えば 1.0 秒から 5.0 秒までかかるリクエストが全て同一視されてしまい、quantile クエリなどが正確でなくなるという問題があった。この変更で、外部から見たい範囲のバケツを細かくするなどの調整ができるようになる。
Checklists
cargo fmt --all.cargo clippy --all --all-targets.