-
Notifications
You must be signed in to change notification settings - Fork 992
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
[Dynatrace v2] Export unit/description to Dynatrace #4006
[Dynatrace v2] Export unit/description to Dynatrace #4006
Conversation
...ons/micrometer-registry-dynatrace/src/main/java/io/micrometer/dynatrace/DynatraceConfig.java
Outdated
Show resolved
Hide resolved
...rometer-registry-dynatrace/src/main/java/io/micrometer/dynatrace/v2/DynatraceExporterV2.java
Outdated
Show resolved
Hide resolved
...rometer-registry-dynatrace/src/main/java/io/micrometer/dynatrace/v2/DynatraceExporterV2.java
Outdated
Show resolved
Hide resolved
…/micrometer/dynatrace/DynatraceConfig.java Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
45de029
to
b596f8f
Compare
I rebased onto main, but it seems I am now conflicting with the dependency lock state. Should I rebase onto an older version or can I fix this some other way? |
Did you rebase onto the upstream |
* @return true if metadata should be exported, false otherwise. | ||
* @since 1.12.0 | ||
*/ | ||
default boolean exportMeterMetadata() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know exactly what the enrichWithDynatraceMetadata()
controls, but I wonder if users will be confused by this exportMeterMetadata()
co-existing with it. What's the motivation to have a configuration method at all for this? Is there a concern of performance issues for users with many meters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enrichWithDynatraceMetadata()
retrieves Dynatrace-specific metadata from the OneAgent, if it's available. This helps Dynatrace put the Micrometer metrics in the right context in our UI.
exportMeterMetadata()
controls whether the metadata set on the meters in Micrometer should be exported at all. There is a performance overhead, and since metadata does not change often, users might want to set it once and then turn off this feature. Dynatrace will remember the metadata you set before. We have seen that turning on this feature can increase the volume of exported data significantly, and want to give users (especially ones that export huge volumes of data) an out. Egress can get quite expensive in certain situations, and exporting redundant data does not help that.
I have thought about other ways to tackle this, but those approaches usually have the downside of having to keep state between exports and would be a much bigger refactoring. I might tackle that at a later point in time. We have users asking for the possibility of exporting metadata, and we want to make sure that users who do not want to have the overhead can opt out.
Do you think adding more documentation on these methods would make it more clear? I will also update the Micrometer docs when this feature is added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think adding more documentation on these methods would make it more clear?
Probably.
I'd point out that users should effectively be able to turn off sending metadata with a MeterFilter that removes the description and unit from meters registered to the DynatraceMeterRegistry. It's always a balance of what to expose as configuration methods that can be done other ways. I'm fine to leave the judgment call on this up to the Dynatrace team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some more context to the Javadocs. I also pointed out that setting that toggle to false is equivalent to configuring a MeterFilter. I think there is value in having it be easy to toggle with a Spring Boot configuration, especially since it might be a somewhat resource-heavy feature.
It is based on a56b968, which is the latest commit in https://github.com/micrometer-metrics/micrometer as of writing. I'll restart them and see if it fixes itself! Edit: The commit (a56b968) on the main branch that works on your CI also seems to fail on our CircleCI instance. I'll have to dig deeper, but I don't know if I'll get around to it today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went on an (extended) wild goose chase to find out why the CI was failing on the Dynatrace fork. The gist of it is: The CI builds (and even local builds for that matter) rely on the git tags published on the Micrometer repo. In our CI (and when I just checked out the forked repo locally), the last tag was 1.7.0-M1. For that reason, all our builds tried to build against 1.7.0-SNAPSHOT:
Inferred project: micrometer, version: 1.7.0-SNAPSHOT
Some time in the last few weeks, a lockfile upgrade led to an incompatibility with 1.7.0:
> Could not resolve all files for configuration ':micrometer-test:java11TestCompileClasspath'.
> Resolved 'org.hdrhistogram:HdrHistogram:2.1.12' which is not part of the dependency lock state
> Resolved 'io.micrometer:micrometer-core:1.9.5' which is not part of the dependency lock state
Copying over the tag for 1.12.0-M2 into our fork fixed all the problems. Now we build against 1.12.0-SNAPSHOT again, and all is right with the world. Don't ask me how much time I spent looking at the two git repos and not understaning why one main branch builds and the other one doesn't when they are at the exact same commit 😅
* @return true if metadata should be exported, false otherwise. | ||
* @since 1.12.0 | ||
*/ | ||
default boolean exportMeterMetadata() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some more context to the Javadocs. I also pointed out that setting that toggle to false is equivalent to configuring a MeterFilter. I think there is value in having it be easy to toggle with a Spring Boot configuration, especially since it might be a somewhat resource-heavy feature.
Sorry you had to go through that, but thank you for getting to the bottom of it, resolving it, and sharing the cause with us. I'll try to keep that in mind should anyone run into a similar issue in the future. |
Co-authored-by: Jonatan Ivanov <jonatan.ivanov@gmail.com>
gh-4006 added support for metrics metadata (unit and description) in the Dynatrace Exporter. The exporter always added the metadata even if unit or description were not set. After these changes, the exporter should not attempt to add metadata if no unit or description was set on the metric. See gh-4247 See gh-3979 See gh-4006 Co-authored-by: Jonatan Ivanov <jonatan.ivanov@gmail.com>
resolves #3979
Metadata (unit and description) can be sent to the Dynatrace metrics API in a special format. This addition will automatically create these lines if unit and/or description are set on the respective Micrometer meter. The feature is turned on by default and can be turned off by toggling the
exportMeterMetadata
switch.