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

Zero percentile can be missing from Dynatrace meters where expected #4750

Open
pirgeo opened this issue Feb 14, 2024 · 10 comments
Open

Zero percentile can be missing from Dynatrace meters where expected #4750

pirgeo opened this issue Feb 14, 2024 · 10 comments
Labels
help wanted An issue that a contributor can help us with registry: dynatrace A Dynatrace Registry related issue

Comments

@pirgeo
Copy link
Contributor

pirgeo commented Feb 14, 2024

Describe the bug
When using the Spring Boot configuration in application.yaml to specify percentiles for Meters, that configuration will overwrite all other configurations.

I am filing this as a bug, but am not sure if this is expected behavior. It is not the behavior I expected, so I know of at least one example where this behavior breaks the DynatraceExporter.

Environment
The bug has been present in Micrometer versions used in Spring Boot for a long time. Due to it only showing up in a situation where percentiles are configured in multiple places, this is a rare one to show up. I know that it was present in 1.9.x, and it is possible to reproduce with versions up to the one on main right now.

  • Micrometer version all released versions
  • Micrometer registry SimpleMeterRegistry, DynatraceMeterRegistry, probably all other registries as well, but I just tested with these two.
  • OS: OS independent
  • Java version: java version independent

To Reproduce
See the reproducer at https://github.com/pirgeo/springboot-percentileconfig. There is a step-by-step example in the readme.

Expected behavior
When using multiple ways of configuring percentiles, I would expect percentile values to be merged, not overwritten.

@shakuzen
Copy link
Member

Thanks for the repro project with steps. I'm not sure there is anything to do in Micrometer related to this. MeterFilters are applied in the order they're registered to a MeterRegistry, which we document here. It's up to the logic of the filter what its behavior is.

This sounds mostly like a Spring Boot issue, and I'm not sure there is anything to do there either but making the documentation more clear. This is the relevant section of the documentation. Looking at MetricsAutoConfiguration you can see that thePropertiesMeterFilter has Order 0 so you can order your MeterFilter beans before or after it. Perhaps that should be documented.

As for the behavior of PropertiesMeterFilter not being additive, I think that's intentional. For your use case, it may be surprising, but for others, it would be surprising that the configuration in application.yml gets combined (potentially with things they don't control). One of the use cases is overriding what is hard-coded in instrumentation that may be in a library you use. Things like percentiles shouldn't be hard-coded in the instrumentation according to best practice, but it could happen. As for merging with other MeterFilters, if you configure things so yours gets registered after PropertiesMeterFilter, you can achieve that.

Does the above make sense? Do you still think something functionally needs to change somewhere? I think this can be closed in favor of improving the Spring Boot documentation (an issue will need to be opened there).

@shakuzen shakuzen added for: external-project For an external project and not something we can fix waiting for feedback We need additional information before we can continue labels Feb 19, 2024
@pirgeo
Copy link
Contributor Author

pirgeo commented Feb 19, 2024

The above makes sense, thanks for the explanation.

The context in which this came up to my attention is the DynatraceMeterRegistry. We require a minimum for certain distribution types, and since this is not tracked by the Micrometer instruments, we resorted to using the 0% quantile to estimate it (#2619). In the method where we add the percentiles, we actually do merge the 0% quantile into the other DistributionStatisticConfig (https://github.com/micrometer-metrics/micrometer/blob/main/implementations/micrometer-registry-dynatrace/src/main/java/io/micrometer/dynatrace/DynatraceMeterRegistry.java#L133-L167). However, it seems as if the DynatraceMeterRegistry-created MeterFilter is ordered in such a way that it gets overwritten by the MeterFilter created by Spring Boot. Do you have an idea of how we can ensure that that doesn't happen?

If there is a way of ensuring that the MeterFilter created by the DynatraceMeterRegistry is applied after the one created by Spring Boot, I think we should be okay. In that case I am happy to close this issue in favor of adding to the Spring Boot docs.

@shakuzen
Copy link
Member

However, it seems as if the DynatraceMeterRegistry-created MeterFilter is ordered in such a way that it gets overwritten by the MeterFilter created by Spring Boot. Do you have an idea of how we can ensure that that doesn't happen?

There's no way to specify ordering in Micrometer other than the order in which MeterFilters are registered. By registering the filter in the constructor for the registry, it is going to end up there before any other filters. It's a bit unusual for a registry to register a filter on itself.

I think maybe taking a step back and looking at the scenario in which this is needed, it seems very specific. It's needed when using the Dynatrace registry, using V2 API, and overriding the default for useDynatraceSummaryInstruments to false. I think the best solution going forward would probably be to deprecate that configuration method and always use Dynatrace implementations of Timer/Summary. Would that be especially breaking for Dynatrace users?

@pirgeo
Copy link
Contributor Author

pirgeo commented Feb 21, 2024

When using the summary instruments, we don't collect percentiles (the summary methods only track min, max, sum, count) so percentile information is lost. This is something users sometimes ask for, so I would like to give them an way to retain that information.
Let me think about that for a little bit, maybe there is a way around that offers the best of both worlds. I have only recently opened another PR to also switch the LongTaskTimers over to the DynatraceSummary collection, since there were a few instances where the data created was inconsistent (estimated min > average or similar, #4724)

Copy link

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@pirgeo
Copy link
Contributor Author

pirgeo commented Feb 29, 2024

I have thought about this a bit more. The best solution here would be to track min/max/sum/count with the Dynatrace instruments to ensure they are consistent for our API, but provide a way to track the percentiles separately. The simplest way of tracking percentiles would be to fall back to the Micrometer's built-in way, but then we lose the performance gain of not having to keep a distribution. In either case, substantial refactoring would be needed, and I don't have the bandwidth to tackle that right now.

As an intermediate solution, if you're using the use-dynatrace-summary-instruments toggle and setting percentiles in your Spring Boot configuration, manually adding the 0 percentile should fix the problem. For example:

management:
  metrics:
    distribution:
      percentiles:
        my.instrument: 0, 0.5, 0.7, 0.9  # make sure you add the `0` here!

The built-in way should work out of the box without the need for explicit changes for all other ways of specifying percentiles.

@pirgeo pirgeo closed this as not planned Won't fix, can't repro, duplicate, stale Feb 29, 2024
@shakuzen
Copy link
Member

For DynatraceMeterRegistry, probably it could be solved by ensuring the 0 percentile is present in the newTimer and newDistributionSummary implementations rather than by registering a MeterFilter.

@pirgeo
Copy link
Contributor Author

pirgeo commented Feb 29, 2024

That's a good point. Shouldn't be too hard to do, I'll see if I can knock it out. It would also be relevant for the LongTaskTimer, for which I have a PR to introduce the Dynatrace Summary instruments open here: #4724.

@pirgeo pirgeo reopened this Feb 29, 2024
@shakuzen shakuzen added registry: dynatrace A Dynatrace Registry related issue and removed for: external-project For an external project and not something we can fix waiting for feedback We need additional information before we can continue labels Mar 1, 2024
@shakuzen shakuzen changed the title Spring Boot distribution config overwrites other ways of specifying percentiles Zero percentile can be missing from Dynatrace meters where expected Mar 1, 2024
@shakuzen shakuzen added the help wanted An issue that a contributor can help us with label Mar 1, 2024
@shakuzen
Copy link
Member

shakuzen commented Mar 1, 2024

I've updated the title to the specific issue discussed.

@pirgeo
Copy link
Contributor Author

pirgeo commented Mar 1, 2024

I've created a PR: #4782
It might be worth waiting until #4724 merges before merging this. We'll need to ensure that the newLongTaskTimer method has the same logic as newTimer and newDistributionSummary. The new DynatraceLongTaskTimer needed for that is introduced in that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted An issue that a contributor can help us with registry: dynatrace A Dynatrace Registry related issue
Projects
None yet
Development

No branches or pull requests

2 participants