This PR contains new additions to the properties that allow Micrometer to export to two different Dynatrace APIs. The code in the two Micrometer PRs (see below) depends on this, and will hopefully be included in the 1.8.0 release of Micrometer.
Resolves #24978
- Micrometer issue: https://github.com/micrometer-metrics/micrometer/issues/2295
- Move Dynatrace exporter v1: https://github.com/micrometer-metrics/micrometer/pull/2553
- Add Dynatrace exporter v2: https://github.com/micrometer-metrics/micrometer/pull/2581
Comment From: snicoll
@pirgeo thank you for the PR but I am going to close this now as it isn't actionable. There is no way for us to review the code as the changes in Micrometer are not available yet. When micrometer has a build with the necessary changes for this PR, we can obviously reconsider.
Comment From: jonatan-ivanov
@snicoll The changes were merged to micrometer: https://github.com/micrometer-metrics/micrometer/pull/2619
Comment From: wilkinsona
Given that there are some configuration properties that are specific to v1 and some that are specific to v2, I wonder if we should introduce separate groups for those. Properties that are common to both version of the API would remain at management.metrics.export.dynatrace while v1-specific properties would move to management.metrics.export.dynatrace.v1 and, similarly, v2-specific properties would move to management.metrics.export.dynatrace.v2. WDYT, @snicoll?
Comment From: snicoll
I think that's an interesting idea. That said our influx support has the same problem and we're currently mapping the same way Micrometer does. I don't know if there's a precedent where our config diverges quite a bit from the related Micrometer's Config class.
Comment From: pirgeo
Would that be a breaking change, or is there a way to do this without breaking existing v1 configurations?
@jonatan-ivanov Have you come across a similar situation and have input here?
Comment From: jonatan-ivanov
@pirgeo On Boot side it depends how the Boot Team wants to handle this. If we can come up with a worthy breaking change for Micrometer, that can be the subject of 2.x. As Stéphane mentioned the Influx config is similar where the v1-v2 is handled similarly to this PR, though I really like Andy's proposal.
Comment From: pirgeo
Thanks @jonatan-ivanov. I also think the proposal makes sense. We would also prefer to introduce this as a non-breaking, additive change. This would allow us to provide a version compatible with Micrometer 1.x without having to wait for a 2.x release. @snicoll, @wilkinsona Would the current proposal in this PR be fine with you?
I have also moved the documentation to the new location. The CI is currently breaking, since this code relies on the 1.8.0-SNAPSHOT build of Micrometer. How can we go ahead here?
Comment From: jonatan-ivanov
@snicoll @wilkinsona Would it make sense to you to depend on Micrometer 1.8.0 in main early on so that Boot support for new features in Micrometer could be merged in?
Comment From: wilkinsona
Yes, I think so. We can do that once we have some dates for the Micrometer 1.8 milestones and we know that the schedules line up.
Comment From: jonatan-ivanov
@wilkinsona We are planning to release M1/M2/M3/RC1/GA 8 days before Boot releases these milestones, e.g.: 1.8.0-M1 is planned on 14th July, 2.6.0-M1 is on 22nd July. Can we include this in 2.6.0-M1?
Comment From: pirgeo
I updated the PR to reflect the renaming of the config option introduced in https://github.com/micrometer-metrics/micrometer/pull/2692
Comment From: jonatan-ivanov
fyi: Micrometer 1.8.0-M1 was released today and saying hi. :)
Comment From: wilkinsona
I've pushed a branch which hopefully illustrates what I had in mind. The approach is to move v1- and v2-specific properties beneath management.metrics.export.dynatrace.v1 and management.metrics.export.dynatrace.v2 respectively. For backwards compatibility, the old v1-specific properties remain at management.metrics.export.dynatrace in a deprecated form.
Rather than having a property for the API version, it is now inferred based on whether or not the device ID has set. A downside of this approach is that a user who is attempting to use the V1 API will not get an error message is they forget to set the device ID. Instead, an attempt will be made to use the V2 API. The upside of the approach is that the IDE experience is better as auto-completion guides you towards the right properties and there's no chance of, for example, setting the API version to v1 and then trying to configure v2-specific properties.
Thoughts?
Comment From: pirgeo
Hi @wilkinsona, Thanks for the suggestion, this is great. We have a small change in mind: would it be possible to rename v1 and v2 to api-v1 and api-v2, respectively? v1/v2 sounds like registry-versions similar to @since where features can be mixed. When using api-v1 or api-v2 it's more clear that we mean the Dynatrace API version, which should clarify that only one of the two (the latest one by default) is used, and features cannot be mixed between versions. What do you think? I have enabled "Allow maintainers to modify" on this PR, so you should be able to push your commit to the open branch if you'd like, otherwise I can take that over. I'll put some finishing touches the docs after we've settled for one of the alternatives
Thanks!
Comment From: wilkinsona
Thanks for taking a look, @pirgeo. We don't use hyphens in configuration property prefixes so we try to keep each .-separated section of the prefix to a single word. We could use apiv1 and apiv2 or v1api and v2api but not api-v1 and api-v2. Personally I prefer the concision of v1 and v2 and don't find it confusing, but you know your user base far better than I do. For v1api vs apiv1 I prefer the former as the latter could be read as apiv 1.
Comment From: pirgeo
@wilkinsona Oh, I see. apiv1 and apiv2 does indeed look a bit odd. Lets go ahead with v1 and v2 then, and I will update the documentation to make it very clear for everyone reading it. Would you push your commit to this branch, or do you want me to do that? Thanks again for the support!
Comment From: wilkinsona
GitHub's giving me a 403 when I try to push to the add-dynatrace-v2-properties branch of dynatrace-oss-contrib/spring-boot. Please feel free to push my commit onto the branch yourself.
Comment From: pirgeo
@wilkinsona I have pushed your commit and refined the documentation, it looks good from our end now. Thanks for your help! I think the build will still break due to this branch pointing to Micrometer 1.7.0, which doesn't have our changes. How should we proceed here?
Comment From: wilkinsona
I can take care of that. I'll rebase it before merging it and that'll pick up the upgrade to 1.8.0-M1.
Comment From: wilkinsona
@pirgeo Thanks very much for making your first contribution to Spring Boot. This has now been merged into main.
Comment From: pirgeo
@wilkinsona Thanks a lot!