micrometer icon indicating copy to clipboard operation
micrometer copied to clipboard

Publishing partial step doesn't work for SignalFx registry

Open lenin-jaganathan opened this issue 2 years ago • 5 comments

Describe the bug https://github.com/micrometer-metrics/micrometer/pull/3681 fixed the issue of publishing partial values when the application is shut down. The issue was addressed by introducing a package-private StepMeter contract which defines _closingRollover() to achieve this. Unfortunately, SignalFxMeterRegistry extends AbstractTimer and AbstractDistributionSummary which are not instances of StepMeter and hence this fix is only applied for Counter type meters.

Environment

  • Micrometer version - 1.11.0
  • Micrometer registry - SignalFx
  • Java version: 8+

To Reproduce How to reproduce the bug: Start a micrometer registry SignalFx

  • step=60seconds
  • and create one timer and one counter. Increment the counter every second and record a random value in the timer every second.

During the app shutdown, the values for the previous step will be reported instead of a closing rollover.

Expected behavior Values accumulated for the current step should be published when the app is shut down.

lenin-jaganathan avatar May 18 '23 04:05 lenin-jaganathan

I think the fixes we did for OTLP (https://github.com/micrometer-metrics/micrometer/pull/3798) are related. There are some lower level components I think we should fix/redesign first to make these work/better (e.g.: better injection for Histogram, interface for Max)

jonatan-ivanov avatar May 18 '23 21:05 jonatan-ivanov

I think the root of this (at least this issue) is because of the inability to StepMeters in SignalFxRegistry. And the reason that SignalFxTimer/Ds didn't extend StepTimer/DS is that Histogram is not pluggable. I guess we addressed the pluggability of Histogram to an extent in this PR which added the capability to provide a custom implementation for Timers. This can be applied to StepTimers too.

By making the SignalFxTimers as StepTimers, I guess micrometer-core will take care of that.

lenin-jaganathan avatar May 19 '23 05:05 lenin-jaganathan

https://github.com/micrometer-metrics/micrometer/pull/3799 fixes this issue for SignalFxMeterRegistry. But there might be issues with StepBucketHistogram cases where Histogram might not get closed properly.

lenin-jaganathan avatar Jun 20 '23 04:06 lenin-jaganathan

Do you have a test / sample that would replicate the issues with Histograms?

marcingrzejszczak avatar Dec 27 '23 15:12 marcingrzejszczak

The "waiting for feedback" label doesn't seem to have worked for some reason.

izeye avatar Feb 22 '24 14:02 izeye

So can we close this?

marcingrzejszczak avatar May 06 '24 09:05 marcingrzejszczak

I think we can close this one.

lenin-jaganathan avatar May 15 '24 07:05 lenin-jaganathan