-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Improve StepBucketHistogram #3793
Improve StepBucketHistogram #3793
Conversation
this(clock, stepMillis, null); | ||
} | ||
|
||
protected StepValue(final Clock clock, final long stepMillis, @Nullable final V initValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be protected? It looks like it could be package-private instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StepBucketHistogram is part of the distribution package so this needs to be protected to use this constructor.
It looks like OTelCollectorIntegrationTest is failing. Perhaps something has changed in a recently published version of the OTel Collector docker image. |
@shakuzen The failures in the OTEL collector test are mostly related to your comment here (at least I think so). Basically, the test expects the timer to have a name like "test_timer_count", but it seems the name is changed to test_timer__count. The same goes for the counter as well, where test_counter is now test_counter_total. Haven't dug in deep but I don't see any related PR's there. |
* eliminate null check on noValue * to support exporting cumulative buckets
2e4f333
to
08c9e43
Compare
@shakuzen / @jonatan-ivanov Does it makes sense for the OtlpStepTimer to be re-named as OtlpDeltaTimer? This is something that I am not able to choose either but I have a slight preference for renaming this as OtlpStepMeters as OtlpDeltaMeter |
...eter-registry-otlp/src/test/java/io/micrometer/registry/otlp/OtlpDeltaMeterRegistryTest.java
Show resolved
Hide resolved
@lenin-jaganathan Thank you very much for the PR! The summary is: our tests depend on the (wrong) behavior of the OTel collector, some of them were fixed which was a breaking change, so our tests were also broken. |
Sorry, I missed this comment while I was reviewing/played with this.
|
This polishes/fixes gh-3749