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

Publish values on close in OTLP registry #3798

Merged

Conversation

jonatan-ivanov
Copy link
Member

The fix for partial publish on close was not applied to OTLP registry.

Closes gh-3773
See gh-1882

@jonatan-ivanov jonatan-ivanov changed the title Publishes values on close in OTLP registry Publish values on close in OTLP registry May 2, 2023
@jonatan-ivanov
Copy link
Member Author

@lenin-jaganathan If you are up to and have some time, would you mind taking a look at this?

@lenin-jaganathan
Copy link
Contributor

@jonatan-ivanov Linking to my comment on the other PR which tries to fix rollover at the top of the minute - #3794 (comment)

It seems this also falls in the same bag of somehow duplicating things from StepRegistry but we can't inherit OTLPRegistry from step registry as it is not just a StepRegistry. Just thinking if we could combine these two PRs and come-up with something better with lesser duplications. cc: @shakuzen

@jonatan-ivanov
Copy link
Member Author

jonatan-ivanov commented May 3, 2023

@lenin-jaganathan This PR is a little more problematic than yours but since we started the discussion there, see my answer there: #3794 (comment)

* class so that {@code OtlpMeterRegistry} can call {@code _closingRollover} on
* {@code StepTuple2} and the method does not need to be public.
*/
class OtlpStepTuple2<T1, T2> extends StepTuple2<T1, T2> {
Copy link
Contributor

@lenin-jaganathan lenin-jaganathan May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that StepTuple2 has been copied over to OTLP. I guess can this support StepN, which can hold count, total, and max, (maybe step-histogram too) so that it makes sure that complete rotation of StepMeter happens with minimal overhead.
Do you think will it be hard to have StepHolder which can hold 'N' items aligned to the same step and should be rotated together?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The concern I have is instead of poll1 and poll2 there should be a poll(index) as well as you should provide a collection of "noValue" and a collection of Supplier.
I think this can be chaotic rather quickly.

I would like to look into improving step-histogram and max (we don't have an interface for that), so that we can make things simpler and more robust. Once we are there, this idea can be handy but right now I think I would prefer separate types.

The fix for partial publish on close was not applied to OTLP registry.

Closes micrometer-metricsgh-3773
See micrometer-metricsgh-1882
@jonatan-ivanov jonatan-ivanov merged commit 234b148 into micrometer-metrics:main May 4, 2023
1 check passed
@jonatan-ivanov jonatan-ivanov deleted the otlp-publish-on-close branch May 4, 2023 21:54
izeye added a commit to izeye/micrometer that referenced this pull request Jun 14, 2023
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.

OTLP registry publishes incorrect delta values on close
2 participants