Avoid global publishing alignment in OTLP registry

[lenin-jaganathan]

This applies gh-3750 for the OtlpDelta flavor. This is more of copying some of the code from StepMeterRegistry for now as this will be critical for the right functionality.

References:

• Avoid global publishing alignment #2818
• Start publish at randomized time in next step #3750

[sonatype-lift]

🛠 Lift Auto-fix

Some of the Lift findings in this PR can be automatically fixed. You can download and apply these changes in your local project directory of your branch to review the suggestions before committing.¹

# Download the patch
curl https://lift.sonatype.com/api/patch/github.com/micrometer-metrics/micrometer/3794.diff -o lift-autofixes.diff

# Apply the patch with git
git apply lift-autofixes.diff

# Review the changes
git diff

Want it all in a single command? Open a terminal in your project's directory and copy and paste the following command:

curl https://lift.sonatype.com/api/patch/github.com/micrometer-metrics/micrometer/3794.diff | git apply

Once you're satisfied, commit and push your changes in your project.

Footnotes

1. You can preview the patch by opening the patch URL in the browser. ↩

[jonatan-ivanov]

@lenin-jaganathan I rebased this to main to fix the build.

[shakuzen]

In general, I still think it'd be best to extend StepMeterRegistry instead of copying all of this over to get the behavior that is already provided by StepMeterRegistry.

[lenin-jaganathan]

In general, I still think it'd be best to extend StepMeterRegistry instead of copying all of this over to get the behavior that is already provided by StepMeterRegistry.

@shakuzen I am really not comfortable with doing this change by extending StepMeterRegistry. This will result in an incorrect relationship, which will say OTLP is a StepRegistry which it is not. Below is the Javadoc for StepMeterRegistry,

Registry that step-normalizes counts and sums to a rate/second over the publishing interval.

However, code duplication also smells equally bad. And we also have #3773 which will most probably have the same code duplication issues. However, I am up for making the PollService and RollOverService as has-a relationship to avoid this duplication.

cc: @jonatan-ivanov

[jonatan-ivanov]

I'm not really keen on extending StepMeterRegistry either and in some cases it does not help a lot I think, in some cases it does but there are trade-offs.
I created a few versions (there are more since they can be combined):

1. I merged together the two PRs (global alignment + final close): main...jonatan-ivanov:micrometer:otlp-push-registry-all-in-one

• This keeps the registry on being a PushMeterRegistry (moving to StepMeterRegistry and then away from it is breaking)
• But it has duplicated code with StepMeterRegistry
• And it makes the _closingRollover method of OtlpStepTuple2 and StepValue protected (I don't think this is a big issue)

2. I also created a version so that OtlpMeterRegistry extends StepMeterRegistry and keeps most of the things similar to the first solution: main...jonatan-ivanov:micrometer:otlp-step-registry-all-in-one

• OtlpMeterRegistry will be a StepMeterRegistry and OtlpConfig will be a StepRegistryConfig, changing this will be breaking
• We should override start since it is different: we should start meterPollingService in case of delta but not in case of cumulative; we can also do this (meterPollingService needs to be changed to protected in StepMeterRegistry and this does not feel good):

if (meterPollingService != null && isCumulative()) {
	this.meterPollingService.shutdownNow();
	this.meterPollingService = null;
}

• pollMetersToRollover is different: takeSnapshot (otlp) is called instead of count (step), also we should have it in the otlp registry at least as a package-private method to make it visible for tests; in order to do this, it needs to be changed to protected in StepMeterRegistry
• getInitialDelay is the same, can be removed
• stop can be removed
• newMeter and newGauge can be removed
• close is slightly different: we should only call closingRollover in case of delta (should not cause issues if called in case of cumulative); also OtlpStepTimer and OtlpStepDistributionSummary need to be handled differently, since they are not StepMeters.
• OtlpStepTuple2 cannot be removed, neither _closingRollover methods (OtlpStepTimer and OtlpStepDistributionSummary)
• histogram in AbstractTimer should stay protected (I needed to change it from private, fyi: it is protected in AbstractDistributionSummary too)
• closingRollover in StepTuple2 and StepValue should stay protected (I needed to change it from package-private)

3. Make StepMeter public and let OtlpStepTimer and OtlpStepDistributionSummary implement it (OtlpMeterRegistry extends StepMeterRegistry): main...jonatan-ivanov:micrometer:otlp-step-registry-and-step-meter

• close can be removed from the registry
• StepMeter is public as well as some _closingRollover methods

4. Utilize StepDistributionSummary and StepTimer: main...jonatan-ivanov:micrometer:otlp-step-registry-and-steptimer-stepsummary

• close can be removed from the registry
• _closingRollover still need to be protected in StepValue
• It has unnecessary fields (max on Otlp and Step Timer/Distribution)

I think another option is to make OtlpMeterRegistry a router and have a OtlpCumulativeMeterRegistry and OtlpStepMeterRegistry but it would not solve most of the problems above, though it would make future changes non-breaking. I also think that redesigning a few things (inject an implementation of Max, better support for injecting Histogram with support of _closingRollover) would make things better and simpler.

[lenin-jaganathan]

@jonatan-ivanov did you try something like,

PollService{
pollsMetersOnStepBoundary();
closingRollOver();
}

And then, StepMeterRegistry will have a has-a relationship with PollService and Otlp can also have that in the case of DeltaFlavour.

I can try this, but I will be out on vacation soon(by the end of this week maybe), so not sure if I can try this before the next release. I also want to fix some of the broken behaviors in SignalFxRegistry too so I bet I will not have enough time to try these things. But just adding them as an idea.

[jonatan-ivanov]

@lenin-jaganathan I haven't tried it. I think the PollService approach is a good idea to make injecting that logic in a more flexible way, though part of the logic is still somewhat duplicated and it also makes closingRollOver public.

Not in this PR but I think we have a bigger issue: we should be able to inject Max and Histogram. Once we can do that OtlpStepTimer and OtlpStepDistributionSummary can extend StepTimer and StepDistributionSummary so that there will be no need for the custom logic (and we can move that out to a separate component that you are suggesting).

My plan is to make changes in this release that have minimal user impact (basically how these PRs are right now) so that we can continue working on this and fix it properly without breaking changes.

Have fun on your vacation! :)