Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tally timer metric #3173

Merged
merged 3 commits into from
Aug 2, 2022
Merged

Fix tally timer metric #3173

merged 3 commits into from
Aug 2, 2022

Conversation

yycptt
Copy link
Member

@yycptt yycptt commented Aug 2, 2022

What changed?

  • Switch back to tally.Timer instead of histogram for timer metrics. The reason is that when using histogram.RecordDuration, if the duration is higher than the max bucket defined, an Inf value will be added to the sum metric emitted and breaks the average calculation. When using histogram.RecordValue, if value is higher than the max bucket defined, no metric will be emitted for the sum metric, and make it inaccurate. Also for RecordValue, the value added to the sum metric is always the defined bucket boundary instead of the actual metric value.
  • tally.Timer implementation has none of the problems mentioned above.
  • However there's no when to specify the buckets when emitting tally.Timer metric, and the default reporter bucket will be used. So wired up the code to specify the default bucket for reporter, which is only used by Timer metric.
  • Bring back the code path for DefaultHistogramBuckets and DefaultHistogramBoundaries so that the code is backward compatible with older versions. Since value defined in ClientConfig.PerUnitHistogramBoundaries was not previously used, it will only be used when neither DefaultHistogramBuckets nor DefaultHistogramBoundaries is specified.
  • Also added more comments for DefaultHistogramBuckets and DefaultHistogramBoundaries config fields.

Why?

  • Fix tally timer metric

How did you test it?

  • Tested locally.

Potential risks
No risks.

Is hotfix candidate?

  • Yes.

@yycptt yycptt requested a review from a team as a code owner August 2, 2022 17:31
@yycptt yycptt merged commit 66852ea into temporalio:master Aug 2, 2022
@yycptt yycptt deleted the fix-tally-timer branch August 2, 2022 19:00
yycptt added a commit that referenced this pull request Aug 2, 2022
alexshtin pushed a commit that referenced this pull request Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants