micrometer
micrometer copied to clipboard
micrometer-metrics
Publish relative to PushMeterRegistry initialization time and align StepMeter boundaries to that
This PR tries to solve this issue (https://github.com/micrometer-metrics/micrometer/issues/2818). The start time is captured when the push meter registry is initialized. For Step based meters, this is passed to the Step Value and StepTupple to align them to the registry start time and not the step start time (see - https://github.com/micrometer-metrics/micrometer/issues/1218).
TODO:
- [ ] rebase and target
1.8.x - [ ] investigate test failures with the
alignToEpochconfig flipped
@lenin-jaganathan Please sign the Contributor License Agreement!
Click here to manually synchronize the status of this Pull Request.
See the FAQ for frequently asked questions.
:warning: 10 God Classes were detected by Lift in this project. Visit the Lift web console for more details.
![sonatype-lift[bot] avatar](https://avatars.githubusercontent.com/in/9843?v=4 "Published by a pub.dev verified publisher"){kind=small}
can you change this PR to draft mode?
@jamesmartinpp Why do you think that should be done? I'm in the process of reviewing it now.
@shakuzen it doesn't really matter. I thought draft mode might be safer. I'm ok proceeding with the PR as is.
After flipping the default, there is a test failure. I've run out of time to dig into it, and I'll be on vacation next week. I can come back to this once I'm back from vacation, but feel free to look into the failure in the meantime.
@shakuzen If the default behavior is made to publish relative to the start time, a few tests will fail because of the way they were written. I will try to address those tests.
Added an additional commit to make the metrics timestamp synchronized across instances(report timestamp as the start of the current step) for SignalFx Meter Registry.
Do you want me to address anything more on this PR?
I don't think so. I'm asking for some review internally. Once that is done, we should be good to go. If you have any thoughts on naming as I've recently updated, please share. Or if you have thoughts on the below.
On how we'll apply this where, we'll keep the default for the configuration false when merging this to main for the next feature release (1.11), which makes the publishing happen not at the same time for every instance. Out of an abundance of caution, now I would like to introduce this in patch releases (1.8.x, 1.9.x, 1.10.x) but initially with the default to true so there is no behavior change unless a user overrides the default. If we get some feedback on usage of this in production, we could consider flipping the default in a patch release later, as the current behavior is very problematic in large deployments. Until we get some feedback on usage, I'm not comfortable making the change in a patch release, even with an option for users to opt out of it. Another thing we're considering is making the configuration option deprecated in 1.11 because as long as there is no problem with the behavior, I'd rather not have a configuration toggle for it going forward (we didn't have one until now because we wanted consistent behavior and there wasn't a need for users to be able to choose). At some point in the future, we could consider removing the configuration altogether.
Putting this in draft mode. Will close this PR in a week. For additional info see the linked issue for why this fix might not be desired.
Fixed by https://github.com/micrometer-metrics/micrometer/pull/3750