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

Follow Micrometer's naming convention #2367

Merged
merged 4 commits into from Jan 6, 2020

Conversation

@trustin
Copy link
Member

trustin commented Jan 3, 2020

Co-authored-by: Per Lundberg perlun@gmail.com

Motivation:

The Micrometer naming conventions (http://micrometer.io/docs/concepts#_naming_meters)
suggest that dotted notation (foo.bar.baz) should be used when naming
meters and tags. This has the advantage of letting different Micrometer
monitoring implementations be able to convert it to the naming
convention which is native for it (e.g. JMX, Prometheus etc).

Modifications:

  • Switch to Micrometer's naming convention everywhere.
  • Add a new flag com.linecorp.armeria.useLegacyMeterNames which
    switches back to the old meter names.
  • Deprecate MoreNamingConventions
    • The NamingConventions returned by MoreNamingConventions now return
      Micrometer's default NamingConvention for corresponding backend.
    • MoreNamingConventions.configure() is now no-op.
  • Miscellaneous:
    • {Dropwizard,Prometheus}MeterRegistries.configureRegistry() does
      not override the pause detector anymore because Micrometer now uses
      NoPauseDetector by default.

Result:

  • Armeria is now a good citizen of Micrometer ecosystem.
  • Users have time to migrate to the new naming thanks to the new flag.
Co-authored-by: Per Lundberg <perlun@gmail.com>

Motivation:

The Micrometer naming conventions (http://micrometer.io/docs/concepts#_naming_meters)
suggest that dotted notation (`foo.bar.baz`) should be used when naming
meters and tags. This has the advantage of letting different Micrometer
monitoring implementations be able to convert it to the naming
convention which is native for it (e.g. JMX, Prometheus etc).

Modifications:

- Switch to Micrometer's naming convention everywhere.
- Add a new flag `com.linecorp.armeria.useLegacyMeterNames` which
  switches back to the old meter names.
- Deprecate `MoreNamingConventions`
  - The `NamingConvention`s returned by `MoreNamingConventions` now return
    Micrometer's default `NamingConvention` for corresponding backend.
  - `MoreNamingConventions.configure()` is now no-op.
- Miscellaneous:
  - `{Dropwizard,Prometheus}MeterRegistries.configureRegistry()` does
    not override the pause detector anymore because Micrometer now uses
    `NoPauseDetector` by default.

Result:

- Armeria is now a good citizen of Micrometer ecosystem.
- Users have time to migrate to the new naming thanks to the new flag.
@trustin

This comment has been minimized.

Copy link
Member Author

trustin commented Jan 3, 2020

@trustin

This comment has been minimized.

Copy link
Member Author

trustin commented Jan 3, 2020

@huydx @linxGnu You might wanna take a look as well.

trustin added 2 commits Jan 3, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 3, 2020

Codecov Report

Merging #2367 into master will decrease coverage by 0.11%.
The diff coverage is 58.82%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2367      +/-   ##
============================================
- Coverage     73.65%   73.54%   -0.12%     
+ Complexity    10573    10569       -4     
============================================
  Files           930      930              
  Lines         40423    40473      +50     
  Branches       4985     5008      +23     
============================================
- Hits          29775    29765      -10     
- Misses         8136     8181      +45     
- Partials       2512     2527      +15
Impacted Files Coverage Δ Complexity Δ
...etrofit2/RetrofitMeterIdPrefixFunctionBuilder.java 100% <ø> (ø) 3 <0> (ø) ⬇️
...meria/common/metric/PrometheusMeterRegistries.java 90.9% <ø> (-0.76%) 5 <0> (ø)
...meria/common/metric/DropwizardMeterRegistries.java 86.11% <ø> (-0.38%) 9 <0> (ø)
...com/linecorp/armeria/common/metric/MoreMeters.java 78.04% <ø> (ø) 9 <0> (ø) ⬇️
...reaker/MetricCollectingCircuitBreakerListener.java 85.71% <0%> (ø) 5 <0> (ø) ⬇️
...ecorp/armeria/common/metric/NoopMeterRegistry.java 73.33% <100%> (ø) 9 <0> (ø) ⬇️
...c/main/java/com/linecorp/armeria/common/Flags.java 62.24% <100%> (+0.31%) 58 <1> (+1) ⬆️
...a/server/composition/AbstractCompositeService.java 78.57% <42.85%> (-6.05%) 12 <0> (ø)
...p/armeria/common/metric/MoreNamingConventions.java 7.4% <42.85%> (-68.6%) 3 <2> (-5)
.../com/linecorp/armeria/server/file/FileService.java 80% <44.44%> (-2.74%) 41 <0> (ø)
... and 15 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 c260bfe...2fa1fe5. Read the comment docs.

@renaudb
renaudb approved these changes Jan 3, 2020
Copy link

renaudb left a comment

LGTM.

@kojilin
kojilin approved these changes Jan 6, 2020
@linxGnu

This comment has been minimized.

Copy link
Member

linxGnu commented Jan 6, 2020

LGTM

@ikhoon
ikhoon approved these changes Jan 6, 2020
Copy link
Contributor

ikhoon left a comment

Nice work, Thanks! 🙇‍♂

meterRegistry.config().namingConvention(MoreNamingConventions.dropwizard());
meterRegistry.config().pauseDetector(new NoPauseDetector());

This comment has been minimized.

Copy link
@ikhoon
@minwoox
minwoox approved these changes Jan 6, 2020
Copy link
Member

minwoox left a comment

👍

@trustin trustin merged commit 8e487fb into line:master Jan 6, 2020
3 of 4 checks passed
3 of 4 checks passed
codecov/patch 58.82% of diff hit (target 73.65%)
Details
codecov/project 73.54% (-0.12%) compared to c260bfe
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@trustin trustin deleted the trustin:follow_micrometer_defaults branch Jan 6, 2020
@trustin

This comment has been minimized.

Copy link
Member Author

trustin commented Jan 6, 2020

Thanks for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.