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

[Dynatrace v2] Export unit/description to Dynatrace #4006

Merged

Conversation

pirgeo
Copy link
Contributor

@pirgeo pirgeo commented Aug 1, 2023

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.

pirgeo and others added 3 commits August 30, 2023 17:45
…/micrometer/dynatrace/DynatraceConfig.java

Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
@pirgeo
Copy link
Contributor Author

pirgeo commented Aug 30, 2023

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?

@shakuzen
Copy link
Member

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 main or on a stale version of main in your fork? I can't reproduce the issue seen with the latest main in this repo (upstream). I couldn't re-run the job either since it is running on CircleCI for your fork.

* @return true if metadata should be exported, false otherwise.
* @since 1.12.0
*/
default boolean exportMeterMetadata() {
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

@pirgeo
Copy link
Contributor Author

pirgeo commented Aug 31, 2023

Did you rebase onto the upstream main or on a stale version of main in your fork?

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.

Copy link
Contributor Author

@pirgeo pirgeo left a 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() {
Copy link
Contributor Author

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.

@shakuzen
Copy link
Member

shakuzen commented Sep 1, 2023

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 😅

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.

@shakuzen shakuzen merged commit 44c334a into micrometer-metrics:main Sep 1, 2023
6 checks passed
@arminru arminru deleted the dynatrace-add-metadata branch September 11, 2023 14:11
izeye added a commit to izeye/micrometer that referenced this pull request Sep 17, 2023
@izeye izeye mentioned this pull request Sep 17, 2023
jonatan-ivanov added a commit to izeye/micrometer that referenced this pull request Sep 20, 2023
Co-authored-by: Jonatan Ivanov <jonatan.ivanov@gmail.com>
jonatan-ivanov added a commit that referenced this pull request Sep 20, 2023
Co-authored-by: Jonatan Ivanov <jonatan.ivanov@gmail.com>
jonatan-ivanov added a commit that referenced this pull request Oct 21, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynatrace Registry: Export units and descriptions
3 participants