[Dynatrace] Add artificial zero percentile in Meter creation rather than a MeterFilter

[pirgeo]

resolves #4750

See the issue for more context. This is mostly a refactoring of where the percentiles are registered (in the MeterFilter vs. in the creation of Meters)

CC @shakuzen

[shakuzen]

Overall looks good, I think. You can rebase now with the changes to LTT. Are you planning to add some tests?

[shakuzen]

Why not use a Set of Id instead of this?

[pirgeo]

I mainly moved the code around and didn't actually change much, but I guess we could use a set of Meter.Ids instead. That would then allow you to explicitly turn on/off the export of the 0th percentile for one particular Meter.Id. For example: meter.name with Tags tag=value1 would export a 0th percentile, but meter.name with tag=value2 would not. I am not sure if this is really that relevant - I assume you generally specify percentiles on the metric name level. I am happy to change it though, if you feel it's relevant.

[pirgeo]

Thanks for taking a look! I rebased on the latest branch and added tests.

[pirgeo]

This comment is referring to these three lines:

micrometer/micrometer-core/src/main/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimer.java

Lines 143 to 145 in e3329b8

	List<Double> percentilesAboveInterpolatableLine = percentilesRequested.stream()
	.filter(p -> p * (activeTasks.size() + 1) > activeTasks.size())
	.collect(Collectors.toList());

They drop the 0 percentile (percentilesAboveInterpolatableLine won't contain the 0 percentile anymore)

I'm not sure if this is a bug or working as intended.

[pirgeo]

I dug a little deeper, and I think the problem lies in the fact that the 0% HistogramGauge is never registered.

1. The LongTaskTimer is registered, with the explicit zero percentile.
2. The histogram gauges get registered. To do so, an initial snapshot is created:
   

   micrometer/micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/HistogramGauges.java

   Line 100 in e3329b8

   HistogramSnapshot initialSnapshot = meter.takeSnapshot();

3. At this point, the number of active tasks is 0. The LongTaskTimer is currently in the process of being registered. Thus, the calculation for percentilesAboveInterpolatableLine looks as follows:
   

   micrometer/micrometer-core/src/main/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimer.java

   Lines 143 to 145 in e3329b8

   	List<Double> percentilesAboveInterpolatableLine = percentilesRequested.stream()
   	.filter(p -> p * (activeTasks.size() + 1) > activeTasks.size())
   	.collect(Collectors.toList());

   
   Since the number of activeTasks is 0, the calculation in the filter effectively evaluates to p * (0 + 1) > 0, which is equivalent to p > 0. This, of course, is a condition that the 0 percentile (where p==0) explicitly does not fulfil. The 0 percentile is dropped here.
4. Because the 0 percentile was dropped in the initial takeSnapshot, it will not be registered as a HistogramGauge because the percentileValues call has only percentiles > 0:

   micrometer/micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/HistogramGauges.java

   Line 103 in e3329b8

   ValueAtPercentile[] valueAtPercentiles = initialSnapshot.percentileValues();

5. Since the 0th percentile gauge is never registered, it is also never exported.

Later, when the app is running and there are active tasks, this is caught in the if here:

micrometer/micrometer-core/src/main/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimer.java

Line 151 in e3329b8

if (!percentilesRequested.isEmpty() || !buckets.isEmpty()) {

However, on the initial takeSnapshot,

micrometer/micrometer-core/src/main/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimer.java

Lines 161 to 165 in e3329b8

	List<Double> youngestToOldestDurations = StreamSupport
	.stream(((Iterable<SampleImpl>) activeTasks::descendingIterator).spliterator(), false)
	.sequential()
	.map(task -> task.duration(TimeUnit.NANOSECONDS))
	.collect(Collectors.toList());

evaluates to an empty list, as there are no active tasks during registration, and the 0th percentile is never registered.

[shakuzen]

We do have at least one bug in this area of the code. See #3877. I'll take a closer look tomorrow.

[shakuzen]

Just wanted to update here that I didn't get a chance to look into it more today due to some other work that needs to get done first. I'll come back to this once that's done.

[pirgeo]

Thank you. I have just very recently seen another example of what seems like a race condition caused by the LongTask ending while the snapshot is being produced. I think Dynatrace should be covered for now with the changes in #4780 (most users will use the new DynatraceLongTaskTimer), but I agree that this should be revisited at some point. Thanks for the heads up!

[shakuzen]

It'll take some more time to look into the LTT issue, I think. But the changes in this pull request don't change the status quo, I think. So nothing is worse related to the bug. I think we're fine to merge these changes and follow-up on #3877 separately. Let me know if I'm missing anything.

[pirgeo]

That's right, there is no change in the status quo, it only fixes a bug where the 0 percent quantile wasn't added automatically when explicitly specifying percentiles.

[pirgeo]

Hi @shakuzen,
I just realized that we never merged this PR, so I'd like to revive it again. I guess I should retarget it against 1.14 now?

[pirgeo]

@shakuzen @jonatan-ivanov Do you think we could revive this one and get it into 1.14? Anything to be done from my side to do so?

[shakuzen]

Sorry about the lack of response. Taking a look now.

[pirgeo]

Thank you for taking a look!