-
Notifications
You must be signed in to change notification settings - Fork 566
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
Implement Micrometer integration, with some new abstractions for interceptors and retrofit to MP metrics #2873
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…th the annotation instead
…cutable (method or constructor); use that from the CDI extension in finding and storing all interception points and (importantly) the metrics to be updated at each; revise some utility methods that retreive annotation info from executables to return more than one instance
…inheritance hierarchy among them
…id leakage from one test to the next
…egistry holding REST.request SimpleTimers
…ns of same metric type appear; existing apps might expect current behavior
…new service-common/rest module
… to the new service-common/rest-cdi module
…date MP metrics to take advantage
…egistering the metrics endpoints
…method but leave it to concrete impl classes to use it to qualify recording producers (rather than always imposing that restriction)
… to concrete impls of HelidonRestCdiExtension, rather than in its observeManagedBeans method
…icrometer to metrics - se/metrics and mp/metrics are where the Micrometer pages are
tjquinno
requested review from
tomas-langer,
spericas,
ljnelson,
romain-grecourt and
ljamen
March 31, 2021 21:53
…e and remove it from the metrics and micrometer subclasses; change MetricsInterceptorBase extend HelidonInterceptor.Base (and remove now-superfluous method impls)
spericas
previously approved these changes
Apr 8, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
tomas-langer
requested changes
Apr 9, 2021
...icrometer/mp/src/main/java/io/helidon/examples/integrations/micrometer/mp/GreetResource.java
Outdated
Show resolved
Hide resolved
...ometer/micrometer/src/main/resources/META-INF/services/javax.enterprise.inject.spi.Extension
Outdated
Show resolved
Hide resolved
...icrometer/micrometer/src/main/java/io/helidon/integrations/micrometer/MicrometerSupport.java
Outdated
Show resolved
Hide resolved
service-common/rest/src/main/java/io/helidon/servicecommon/rest/HelidonRestServiceSupport.java
Show resolved
Hide resolved
…ad of io.helidon.integrations) for the Micrometer REST and CDI integrations
tomas-langer
approved these changes
Apr 13, 2021
aseovic
pushed a commit
to aseovic/helidon
that referenced
this pull request
Apr 26, 2021
…rceptors and retrofit to MP metrics (helidon-io#2873)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #2344.
This is a re-work of #2761 which is now obsolete.
Although many files are affected, I used the first few commits to "chunk" the changes into more reasonably reviewable sections. This PR has evolved in parallel with #2868, including some rebases. The original chunks here are still conceptually valid although some things (like renaming some parameters and classes) appear later in the PR history.
There would have been considerable overlap between MP metrics and Micrometer code. Much of this PR abstracts common behavior out into interfaces and classes in the new
service-common/rest
andservice-common/rest-cdi
components and then makes use of those from SE and MP metrics and the Micrometer integration.PR #2868 abstracted quite a bit of common behavior for interceptors into separate classes and interfaces. This PR:
What follows is a much longer description of the work in this PR.
A Possibly Digestible Guided Review
This PR is similar to its earlier incarnation as #2761.
Because the original metrics SE and MP components share so much with their new Micrometer counterparts, this PR combined with the earlier MP-only PR #2868 (which added some abstracting and async support) together have three missions:
service-common
modules,Abstractions
Look at the new
service-common
modulesrest
andrest-cdi
.service-common/rest
Contains
HelidonRestServiceSupport
, an abstract superclass forxxxSupport
SE classes. It handles config-based customization of the service’s endpoint usingweb-context
and sets up and configures CORS for the endpoint.service-common/rest-cdi
HelidonRestCdiExtension
- abstract superclass for CDI extensions which themselves usexxxSupport
classes. Helps in creating the underlyingxxxSupport
service, discarding vetoed types or beans from annotation processing, and more.HelidonInterceptor
- interface applicable to interceptors in many components - Has severaldefault
methods. Provides both “before-only” and “before-and-after” semantics around intercepted invocations.InterceptionRunner
and its implementation - Not used outside the package, but used byHelidonInterceptor
to invoke the distinct behavior in the interceptors and invoke the interceptedExecutable
. Automatically supports asynchronous JAX-RS endpoints, invoking each interceptor’s post-completion action only after the async work has completed.Those of you who reviewed the earlier MP-only PR will notice that the public
HelidonRestCdiExtension
andHelidonInterceptor
expose a smaller and simpler surface area than their predecessors from the earlier PR. The runner interface and implementation are now invisible outside the package.The evolved metrics components
metrics/metrics
- The SEMetricsSupport
class has migrated to use the new abstraction.microprofile/metrics
- The MP CDI extension is shorter and the interceptors are much simpler (and should be faster, and also honor any removals or adjustments to the annotations that other CDI extensions might have made).The new
integrations/micrometer
components/micrometer
(SE) for the newMicrometerSupport
class/cdi
(MP) for the new extension and interceptorsOther pieces
The Helidon Micrometer implementation largely follows the plan laid out in this gist: https://github.com/oracle/helidon/wiki/Options-for-Micrometer-in-Helidon (see the “short-term” option).