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

Add support for Dynatrace metrics v2 API #26258

Conversation

pirgeo
Copy link
Contributor

@pirgeo pirgeo commented Apr 27, 2021

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

@snicoll
Copy link
Member

snicoll commented Apr 28, 2021

@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.

@snicoll snicoll closed this Apr 28, 2021
@snicoll snicoll removed the status: waiting-for-triage An issue we've not yet triaged label Apr 28, 2021
@jonatan-ivanov
Copy link
Member

@snicoll The changes were merged to micrometer: micrometer-metrics/micrometer#2619

@wilkinsona wilkinsona reopened this Jun 9, 2021
@wilkinsona wilkinsona added the status: waiting-for-triage An issue we've not yet triaged label Jun 9, 2021
----

You can also change the interval at which metrics are sent to Dynatrace:
You can also change the interval at which metrics are sent to Dynatrace (works for both API versions).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this applies to both API versions, I don't think it belongs in the API v1 section.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I added a subsection for version independent settings.

@snicoll snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 14, 2021
@snicoll snicoll added this to the 2.6.x milestone Jun 14, 2021
@wilkinsona
Copy link
Member

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?

@snicoll
Copy link
Member

snicoll commented Jun 14, 2021

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.

@pirgeo
Copy link
Contributor Author

pirgeo commented Jun 15, 2021

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?

@snicoll snicoll changed the title Add properties for Dynatrace metrics API v2 ingest with Micrometer Add support for Dynatrace metrics API v2 Jun 16, 2021
@jonatan-ivanov
Copy link
Member

@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.

@pirgeo pirgeo force-pushed the add-dynatrace-v2-properties branch from ff8077d to 692decd Compare June 17, 2021 09:58
@pirgeo
Copy link
Contributor Author

pirgeo commented Jun 17, 2021

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?

@jonatan-ivanov
Copy link
Member

@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?

@wilkinsona
Copy link
Member

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.

@wilkinsona
Copy link
Member

wilkinsona commented Jul 15, 2021

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?

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Jul 15, 2021
@pirgeo
Copy link
Contributor Author

pirgeo commented Jul 16, 2021

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!

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jul 16, 2021
@wilkinsona
Copy link
Member

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.

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jul 16, 2021
@pirgeo
Copy link
Contributor Author

pirgeo commented Jul 16, 2021

@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!

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jul 16, 2021
@wilkinsona
Copy link
Member

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.

@pirgeo pirgeo force-pushed the add-dynatrace-v2-properties branch from 4090f20 to 4cdea7b Compare July 16, 2021 15:05
@pirgeo
Copy link
Contributor Author

pirgeo commented Jul 16, 2021

@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?

@wilkinsona
Copy link
Member

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.

wilkinsona pushed a commit that referenced this pull request Jul 16, 2021
wilkinsona added a commit that referenced this pull request Jul 16, 2021
@wilkinsona
Copy link
Member

@pirgeo Thanks very much for making your first contribution to Spring Boot. This has now been merged into main.

@wilkinsona wilkinsona modified the milestones: 2.6.x, 2.6.0-M1 Jul 16, 2021
@wilkinsona wilkinsona changed the title Add support for Dynatrace metrics API v2 Add support for Dynatrace metrics v2 API Jul 16, 2021
@wilkinsona wilkinsona added status: noteworthy A noteworthy issue to call out in the release notes and removed status: feedback-provided Feedback has been provided labels Jul 16, 2021
@pirgeo
Copy link
Contributor Author

pirgeo commented Jul 16, 2021

@wilkinsona Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: noteworthy A noteworthy issue to call out in the release notes type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add auto-configuration for Micrometer's Dynatrace v2 meter registry
5 participants