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

Make Dropwizard Metrics and Prometheus optional for Spring Boot integration #2107

Merged
merged 6 commits into from
Sep 25, 2019

Conversation

trustin
Copy link
Member

@trustin trustin commented Sep 24, 2019

Motivation:

A user may not want to pull in Dropwizard Metrics or Prometheus at all.

Modifications:

  • Access anything related with Dropwizard or Prometheus only when the related classes are available at runtime.
  • Make io.dropwizard.metrics:metrics-json and io.micrometer:micrometer-registry-prometheus optional.

Result:

Verified

This commit was signed with the committer’s verified signature.
trustin Trustin Lee
Motivation:

A user may not want to pull in Dropwizard Metrics at all.

Modifications:

- Access anything related with Dropwizard using the reflection API.
- Make `io.dropwizard.metrics:metrics-json` optional.

Result:

- Fixes line#2106
- A user can exclude `io.dropwizard.metrics:metrics-core` as well as
  `metrics-json`.
@trustin
Copy link
Member Author

trustin commented Sep 24, 2019

@adriancole PTAL. I think now you can even exclude metrics-core, unless Micrometer does something interesting.

@codefromthecrypt
Copy link
Contributor

testing now.. the code looks correct

codefromthecrypt pushed a commit to openzipkin/zipkin that referenced this pull request Sep 24, 2019
@codefromthecrypt
Copy link
Contributor

verified openzipkin/zipkin#2818 (comment)

Verified

This commit was signed with the committer’s verified signature.
trustin Trustin Lee
@codecov
Copy link

codecov bot commented Sep 24, 2019

Codecov Report

Merging #2107 into master will decrease coverage by 0.05%.
The diff coverage is 50.98%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2107      +/-   ##
============================================
- Coverage     73.71%   73.66%   -0.06%     
- Complexity     9466     9471       +5     
============================================
  Files           831      833       +2     
  Lines         36495    36513      +18     
  Branches       4516     4521       +5     
============================================
- Hits          26903    26898       -5     
- Misses         7273     7293      +20     
- Partials       2319     2322       +3
Impacted Files Coverage Δ Complexity Δ
...orp/armeria/internal/spring/DropwizardSupport.java 13.33% <13.33%> (ø) 2 <2> (?)
...orp/armeria/internal/spring/PrometheusSupport.java 66.66% <66.66%> (ø) 3 <3> (?)
...eria/internal/spring/ArmeriaConfigurationUtil.java 73.54% <66.66%> (+1.54%) 47 <2> (+1) ⬆️
...rp/armeria/server/tomcat/ManagedTomcatService.java 50.84% <0%> (-22.04%) 6% <0%> (-1%)
...armeria/server/tomcat/Tomcat90ProtocolHandler.java 48.14% <0%> (-14.82%) 11% <0%> (-4%)
.../linecorp/armeria/internal/Http2ObjectEncoder.java 71.42% <0%> (-8.58%) 12% <0%> (-1%)
...meria/internal/AbstractHttp2ConnectionHandler.java 93.33% <0%> (-3.34%) 13% <0%> (-1%)
.../linecorp/armeria/server/tomcat/TomcatService.java 63.82% <0%> (-2.66%) 24% <0%> (-1%)
.../linecorp/armeria/internal/Http2GoAwayHandler.java 53.84% <0%> (-2.57%) 13% <0%> (-1%)
...necorp/armeria/server/GracefulShutdownSupport.java 97.43% <0%> (-2.57%) 4% <0%> (ø)
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42c29ad...10c1dd3. Read the comment docs.

@trustin trustin changed the title Make Dropwizard Metrics optional Make Dropwizard Metrics and Prometheus optional for Spring Boot integration Sep 24, 2019

Verified

This commit was signed with the committer’s verified signature.
trustin Trustin Lee

Verified

This commit was signed with the committer’s verified signature.
trustin Trustin Lee

Verified

This commit was signed with the committer’s verified signature.
trustin Trustin Lee
Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

retested.. still works!

Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

👍

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@trustin trustin merged commit 3959fc3 into line:master Sep 25, 2019
@trustin trustin deleted the spring_boot_optional_dropwizard branch September 25, 2019 04:48
@trustin
Copy link
Member Author

trustin commented Sep 25, 2019

Thanks for revieweing!

eugene70 pushed a commit to eugene70/armeria that referenced this pull request Oct 16, 2019
…ration (line#2107)

Motivation:

A user may not want to pull in Dropwizard Metrics at all.

Modifications:

- Access anything related with Dropwizard using the reflection API.
- Make `io.dropwizard.metrics:metrics-json` optional.

Result:

- Fixes line#2106
- A user can exclude `io.dropwizard.metrics:metrics-core` as well as
  `metrics-json`.
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
…ration (line#2107)

Motivation:

A user may not want to pull in Dropwizard Metrics at all.

Modifications:

- Access anything related with Dropwizard using the reflection API.
- Make `io.dropwizard.metrics:metrics-json` optional.

Result:

- Fixes line#2106
- A user can exclude `io.dropwizard.metrics:metrics-core` as well as
  `metrics-json`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accidental strict dep on codahale metrics
5 participants