[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Henry Robinson has uploaded a new change for review. http://gerrit.cloudera.org:8080/4516 Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. IMPALA-4187: Switch RPC latency metrics to histograms It's usually better to measure latency distributions with histograms, not avg / min / max. This change switches out the metrics that measure the latency of RPC processing time to histograms. Add HistogramMetric::Reset() to allow histogram to remove all entries. Added spinlock around histogram access. Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 --- M be/src/rpc/rpc-trace.cc M be/src/rpc/rpc-trace.h M be/src/util/histogram-metric.h 3 files changed, 58 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/4516/2 -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson
[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Henry Robinson has posted comments on this change. Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. Patch Set 2: The core change to HistogramMetric (adding a Reset() method) is needed for our proposed solution to https://gerrit.cloudera.org/#/c/4500/. Since I had this patch all ready to go, might as well get the RPC metric changes reviewed at the same time. -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Juan Yu has posted comments on this change. Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/4516/2/be/src/rpc/rpc-trace.cc File be/src/rpc/rpc-trace.cc: PS2, Line 181: SIXTY_MINUTES_IN_MS If I understand correctly, this means the longest RPC call could be 60min. Isn't it too long? most of RPC should be in ms range or even shorter, 1 min is already abnormal, right? http://gerrit.cloudera.org:8080/#/c/4516/2/be/src/util/histogram-metric.h File be/src/util/histogram-metric.h: PS2, Line 37: highest_trackable_value, num_significant_digits would it be worth to store those in HistogramMetric? -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Juan Yu has posted comments on this change. Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. Patch Set 2: What's the performance of updating this metrics? -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/4516/2/be/src/rpc/rpc-trace.cc File be/src/rpc/rpc-trace.cc: PS2, Line 179: int32_t uint32_t? PS2, Line 181: 3 nit: I've previously learned that it's a good practice not to use "magic" constants such as used here, or if it's absolutely necessary, a small comment should explain what the use of said constant is. Is that something that's worth following? http://gerrit.cloudera.org:8080/#/c/4516/2/be/src/util/histogram-metric.h File be/src/util/histogram-metric.h: Line 44: There's no shared state access before this line. Worth locking from here? Line 58: And releasing the lock here? PS2, Line 70: histogram_->Increment(val); On an unrelated note, I looked through the implementation of HdrHistogram and there seems to be use of a lot of atomic operations, but that doesn't make it race-free (which is why it makes sense to use a lock here). Any idea why HdrHistogram is written that way? (Not super important. I was just curious; if you're not sure, we can always revisit it later) PS2, Line 107: boost::scoped_ptr histogram_; I know we had a scoped_ptr vs unique_ptr discussion thread recently, but it doesn't seem like we reached a verdict. It doesn't seem like we intend to move() or release() this pointer, but are we okay with increasing dependency on boost by adding new scoped_ptrs? -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Henry Robinson has posted comments on this change. Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. Patch Set 2: (8 comments) Perf results are in the commit msg now. http://gerrit.cloudera.org:8080/#/c/4516/2/be/src/rpc/rpc-trace.cc File be/src/rpc/rpc-trace.cc: PS2, Line 179: int32_t > uint32_t? We typically don't use uint (not consistent in that, unfortunately). PS2, Line 181: 3 > nit: I've previously learned that it's a good practice not to use "magic" c Happy to add a comment. PS2, Line 181: SIXTY_MINUTES_IN_MS > If I understand correctly, this means the longest RPC call could be 60min. Sure, but what's the drawback of having 60 minutes be the upper bound? http://gerrit.cloudera.org:8080/#/c/4516/2/be/src/util/histogram-metric.h File be/src/util/histogram-metric.h: PS2, Line 37: highest_trackable_value, num_significant_digits > would it be worth to store those in HistogramMetric? What would be the benefit? They're already stored in HdrHistogram. Line 44: > There's no shared state access before this line. Worth locking from here? Done Line 58: > And releasing the lock here? Done PS2, Line 70: histogram_->Increment(val); > On an unrelated note, I looked through the implementation of HdrHistogram a I'm not sure I follow - HdrHistogram is thread-safe (at least per the class comment). The reason we have locks here is to protect access to the pointer to the histogram object. PS2, Line 107: boost::scoped_ptr histogram_; > I know we had a scoped_ptr vs unique_ptr discussion thread recently, but it Yes, I think that was the result of the conversation so far. scoped_ptr for objects whose ownership will never change. -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Henry Robinson has uploaded a new patch set (#3). Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. IMPALA-4187: Switch RPC latency metrics to histograms It's usually better to measure latency distributions with histograms, not avg / min / max. This change switches out the metrics that measure the latency of RPC processing time to histograms. Add HistogramMetric::Reset() to allow histogram to remove all entries. Added spinlock around histogram access. On a 8-core i7 @ 3.4GHz, the following throughputs were observed: 1 thread -> 25M updates/sec 4 threads -> 7M updates/sec 16 threads -> 5M updates/sec Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 --- M be/src/rpc/rpc-trace.cc M be/src/rpc/rpc-trace.h M be/src/util/histogram-metric.h 3 files changed, 61 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/4516/3 -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Juan Yu has posted comments on this change. Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/4516/2/be/src/rpc/rpc-trace.cc File be/src/rpc/rpc-trace.cc: PS2, Line 181: SIXTY_MINUTES_IN_MS > Sure, but what's the drawback of having 60 minutes be the upper bound? More buckets and HdrHistogram::ValueAtPercentile() will take longer time. But we don't update metrics that often so it's ok. -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/4516/2/be/src/util/histogram-metric.h File be/src/util/histogram-metric.h: PS2, Line 70: id Update(int64_t val) { > I'm not sure I follow - HdrHistogram is thread-safe (at least per the class Nvm, looked at it more carefully again, and it is lock-free but thread safe. -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Juan Yu has posted comments on this change. Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. Patch Set 3: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/4516/3/be/src/rpc/rpc-trace.cc File be/src/rpc/rpc-trace.cc: PS3, Line 179: constexpr int32_t SIXTY_MINUTES_IN_MS = 60 * 1000 * 60; We'll see users with long running queries go over this (not uncommon) for hs2::ExecuteStatement/beeswax::query I'm not sure it's worth doing anything about, but do you know how it will be handled? -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Henry Robinson has posted comments on this change. Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/4516/3/be/src/rpc/rpc-trace.cc File be/src/rpc/rpc-trace.cc: PS3, Line 179: constexpr int32_t SIXTY_MINUTES_IN_MS = 60 * 1000 * 60; > We'll see users with long running queries go over this (not uncommon) for h I checked - any value > 60 minutes will be clipped to 60 minutes. That seems ok - beyond 60 minutes I'm not sure there's much use in knowing the distribution. 60 minutes might be too much since most RPCs will never get anywhere close, but the total data structure cost is 100KB and there are only a handful of them for all the RPCs we support. -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. Patch Set 3: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/4516/3/be/src/rpc/rpc-trace.cc File be/src/rpc/rpc-trace.cc: PS3, Line 179: constexpr int32_t SIXTY_MINUTES_IN_MS = 60 * 1000 * 60; > I checked - any value > 60 minutes will be clipped to 60 minutes. That seem wfm, thanks for checking. -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Dan Hecht has posted comments on this change. Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. Patch Set 3: Code-Review+2 (1 comment) Can you give an example of what the old and new metries look like? Does CM need to be updated at all? http://gerrit.cloudera.org:8080/#/c/4516/3/be/src/util/histogram-metric.h File be/src/util/histogram-metric.h: Line 107: // Protects histogram_ how about: Protects histogram_ pointer. to make it clear that it's only the pointer, not the implementation that's being protected. -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Matthew Jacobs has posted comments on this change. Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. Patch Set 3: > (1 comment) > > Can you give an example of what the old and new metries look like? > Does CM need to be updated at all? CM doesn't collect these metrics yet (I checked their code before), but this reminds me we should update common/thrift/metrics.json The rpc-method { "description": "Duration (ms) of RPC calls to $0", "contexts": [ "CATALOGSERVER", "STATESTORE", "IMPALAD" ], "label": "$0 RPC Call Duration", "units": "TIME_MS", "kind": "STATS", "key": "rpc-method.$0.call_duration" }, STATS -> HISTOGRAM I expected there to be a runtime check to catch this mistake, but it looks like we check the metric "type" in SimpleMetric and not in HistogramMetric. The histogram metric constructor should DCHECK_EQ(TMetricKind::HISTOGRAM, metric_def.kind); -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Hello Juan Yu, Matthew Jacobs, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4516 to look at the new patch set (#4). Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. IMPALA-4187: Switch RPC latency metrics to histograms It's usually better to measure latency distributions with histograms, not avg / min / max. This change switches out the metrics that measure the latency of RPC processing time to histograms. Add HistogramMetric::Reset() to allow histogram to remove all entries. Added spinlock around histogram access. On a 8-core i7 @ 3.4GHz, the following throughputs were observed: 1 thread -> 25M updates/sec 4 threads -> 7M updates/sec 16 threads -> 5M updates/sec Each histogram takes ~108KB of storage for its buckets. This can be reduced by reducing the maximum value, currently 60 minutes. The new metrics have the following text representation: Count: 148, 25th %-ile: 0, 50th %-ile: 0, 75th %-ile: 0, 90th %-ile: 0, 95th %-ile: 0, 99.9th %-ile: 1ms Which changes from the old: count: 345, last: 6ms, min: 0, max: 12ms, mean: 1ms, stddev: 1ms Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 --- M be/src/rpc/rpc-trace.cc M be/src/rpc/rpc-trace.h M be/src/util/histogram-metric.h M common/thrift/metrics.json 4 files changed, 64 insertions(+), 37 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/4516/4 -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Henry Robinson has posted comments on this change. Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. Patch Set 3: (1 comment) Added before and after to the commit msg. http://gerrit.cloudera.org:8080/#/c/4516/3/be/src/util/histogram-metric.h File be/src/util/histogram-metric.h: Line 107: // Protects histogram_ > how about: Protects histogram_ pointer. Done -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Henry Robinson has posted comments on this change. Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. Patch Set 4: Code-Review+2 Carry +2, made Matt's suggested changes as well. -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Internal Jenkins has posted comments on this change. Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. Patch Set 4: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4187: Switch RPC latency metrics to histograms
Internal Jenkins has submitted this change and it was merged. Change subject: IMPALA-4187: Switch RPC latency metrics to histograms .. IMPALA-4187: Switch RPC latency metrics to histograms It's usually better to measure latency distributions with histograms, not avg / min / max. This change switches out the metrics that measure the latency of RPC processing time to histograms. Add HistogramMetric::Reset() to allow histogram to remove all entries. Added spinlock around histogram access. On a 8-core i7 @ 3.4GHz, the following throughputs were observed: 1 thread -> 25M updates/sec 4 threads -> 7M updates/sec 16 threads -> 5M updates/sec Each histogram takes ~108KB of storage for its buckets. This can be reduced by reducing the maximum value, currently 60 minutes. The new metrics have the following text representation: Count: 148, 25th %-ile: 0, 50th %-ile: 0, 75th %-ile: 0, 90th %-ile: 0, 95th %-ile: 0, 99.9th %-ile: 1ms Which changes from the old: count: 345, last: 6ms, min: 0, max: 12ms, mean: 1ms, stddev: 1ms Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Reviewed-on: http://gerrit.cloudera.org:8080/4516 Reviewed-by: Henry Robinson Tested-by: Internal Jenkins --- M be/src/rpc/rpc-trace.cc M be/src/rpc/rpc-trace.h M be/src/util/histogram-metric.h M common/thrift/metrics.json 4 files changed, 64 insertions(+), 37 deletions(-) Approvals: Henry Robinson: Looks good to me, approved Internal Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/4516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Juan Yu Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Sailesh Mukil