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

Update Spring Boot example config for Dynatrace #2658

Conversation

arminru
Copy link
Contributor

@arminru arminru commented Jun 18, 2021

No description provided.

@arminru
Copy link
Contributor Author

arminru commented Jun 18, 2021

Pending on spring-projects/spring-boot#26258.

@shakuzen shakuzen added registry: dynatrace A Dynatrace Registry related issue type: task A general task labels Jun 21, 2021
@shakuzen shakuzen added this to the 1.8 backlog milestone Jun 21, 2021
Copy link
Member

@jonatan-ivanov jonatan-ivanov left a comment

Choose a reason for hiding this comment

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

I'm thinking if we should keep the original V1 samples too:

  • Rename DynatraceSample to DynatraceV1Sample
  • Reaname application-dynatrace.yml to application-dynatrace-v1.yml
  • Modify the profile in DynatraceV1Sample to dynatrace-v1
  • Copy DynatraceV1Sample to DynatraceV2Sample and add profile dynatrace-v2
  • Use the config in this PR and create application-dynatrace-v2.yml

WDYT?

@arminru
Copy link
Contributor Author

arminru commented Jun 23, 2021

@jonatan-ivanov These examples are intended as a starting point for new users, right? We should encourage them to use the new v2 registry/API right away rather than starting with the old one at this point. If that would be fine with you, I'd prefer changing the example rather than having both in there. Otherwise your suggested changes would sound alright to me. Let me know what you think. Also, @pirgeo will take over this PR as I'm on vacation.

@jonatan-ivanov
Copy link
Member

@arminru Have a great vacation! :)

@arminru @pirgeo
I think the main intention of the examples is to show a working demo and to help the users.
Since the V1 registry is still available and it is the default, I think it would be a good idea to keep the V1 example around.

I totally get the intention of encouraging V2. What do you think about putting a comment both to the v1 .properties and .java files that is saying something like the Dynatrace Team recommends using the V2 API?

We can also put a WARN log into the V1 example app.

@pirgeo
Copy link
Contributor

pirgeo commented Jul 2, 2021

@jonatan-ivanov I played around with that for a bit, but hit a bump: It seems to be forbidden by the checkstyle command to import org.slf4j.* in the example classes - Is there a way of printing a warning from the example app?

@jonatan-ivanov
Copy link
Member

jonatan-ivanov commented Jul 2, 2021

Ouch, I see:

[ant:checkstyle] [ERROR] .../samples/DynatraceSampleV1.java:19:1: Illegal import - org.slf4j.LoggerFactory. [IllegalImport]

A couple of alternatives:

  1. InternalLoggerFactory.getInstance(DynatraceSampleV1.class)
    The problem with this is that users should not use it, so I would not advertise it in that sample
  2. System.out.println(); or System.err.println();
    This actually is not as bad as it sounds since you don't need any features that a logger has and IDEs show the error stream in different color so it will be more apparent (you can also put it into some frame like ---, === if you want to)
  3. //CHECKSTYLE:OFF and //CHECKSTYLE:ON 😈 , see: SuppressionCommentFilter

Here's an example for the third option (I reverted your log removal and merged back main): jonatan-ivanov@64e588a

I think I would just go with System.err.println(); because that is the most apparent and the simplest.

@arminru
Copy link
Contributor Author

arminru commented Jul 8, 2021

@jonatan-ivanov Your suggestions sound good but for now we decided to be less explicit and just added comments indicating that v2 is preferred rather than printing warnings right away. We can always make it more explicit in future.

@arminru arminru marked this pull request as ready for review July 8, 2021 11:55
@jonatan-ivanov jonatan-ivanov merged commit 694837f into micrometer-metrics:main Jul 8, 2021
@jonatan-ivanov
Copy link
Member

Sounds good, thank you for adding the example.

@jonatan-ivanov jonatan-ivanov modified the milestones: 1.8 backlog, 1.8.0-M1 Jul 8, 2021
@arminru arminru deleted the dynatrace-example branch July 9, 2021 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
registry: dynatrace A Dynatrace Registry related issue type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants