-
Notifications
You must be signed in to change notification settings - Fork 565
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
Metrics 4.0 implementation #3847
Conversation
…or new files in this PR given the new year
… Add dep on config yaml parser for examples (no longer dragged in automatically)
…e interceptor runs, to avoid race condition; add test
metrics/api/src/main/java/io/helidon/metrics/api/MetricStore.java
Outdated
Show resolved
Hide resolved
Converted to draft because of an intermittent failure in the TCK and in a |
…processing, not immediately after the intercepted method is invoked (to account properly for async requests)
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
There should be a certain ordering of fields in a class. Some of the constants have changed visibility (from private to package private). I did not mark this as a request for change, as we would get a complicated diff. Maybe as a follow up, it would be good to reorder the elements (without other changes).
Resolves #2644
Note: There are no obvious substantive changes required to the Helidon doc for metrics as a result of these changes. Some of the example output will change because of added attributes to some of the types of metrics. But this PR does not really introduce new Helidon metrics behavior apart from what's required by the newer spec versions (3.0 and 4.0). I will add the minimal doc changes to this PR shortly.
This intro is long but will help reviewers understand the changes in the many revised files.
Overview
These changes implement updates from MP metrics 3.0 (significant changes previously not incorporated into Helidon) and 4.0
(Jakarta namespace change).
General notes
MP Metrics 3.0 did away with the distinction between reusable and non-reusable metrics; all are reusable. I could discard quite a bit of code devoted to that difference.
The spec, and related MP metrics issue and PR discussions, make clear that if an application introduces CDI producers for metrics, the app developer must use CDI qualifiers (for example) to disambiguate those producers from ones provided by the MP metrics implementation itself. Helidon's impl had added a synthetic
@VendorDefined
qualifier to every observed injection point to help hide the need for disambiguation from developers. With the clarification in the spec, we no longer need to do that so this PR removes that qualifer and all references to it. I added tests which include app-defined producers with qualifiers to make sure we handle that case.The automatically-recorded metrics named
REST.request
(which measure all REST requests) now distinguish between successfully-processed requests (which either complete successfully or have app-provided exception mappers which apply) vs. failed requests (which end with an unmapped exception). There are several changes to handle this described below.In several classes,
synchronized
methods and/orAtomicXXX
s are replaced with a read-write lock to control shared access to scalars.In connection with this, some private methods have the name
xxxLocked
to indicate that they access shared data but must be invoked only after the caller has obtained the correct type of lock (read or write).Methods without such a suffix either do not need to arbitrate shared access to data or obtain it themselves.
In several public classes, protected methods or fields lacked JavaDoc which triggered JavaDoc warnings. I added JavaDoc in several such places to silence the warnings.
Concurrent changes to this PR added support for metrics settings and registry settings (POJO analogs to config settings for metrics as a whole and specific registries). Some of the changes in this PR take care of that and I do not discuss those changes further below.
I don't list changes to tests for new features required by the spec; I mention the spec changes briefly with the classes where the features are implemented.
I don't discuss every change or every single revised file when the changes are mostly self-explanatory.
Changes to components
helidon-metrics-api
(metrics/api
)We use multiple data structures to hold registered metrics and their metadata and tags. To simplify that a bit and to improve shared access to the shared data structures, the new
MetricStore
class now abstracts all those details.AbstractRegistry
still defines the API used internally by Helidon metrics, but now it largely delegates to
MetricStore` for operations which update or query the data structures.As part of this, synchronization in
MetricStore
does not use thesynchronized
keyword on methods but rather uses aReadWriteLock
to control access to the shared data structures. The data structures and how the code uses them has not changed from previous releases. They exist largely to optimize certain queries either required by the spec APIs or useful in our implementation.The MP metrics 3.0 spec added several new methods to
MetricRegistry
which are reflected inAbstractRegistry
.Many of these had been added as no-ops to get early builds to work, and the no-ops have been replaced by proper implementations.
Although
MetricSettings
predates this PR, it gains two new attributes. A long-standing issue with MP metrics was that creating a newMetricID
always triggered config look-ups for global tags set in config or an application identifier, also set in config. MP Metrics 3.0 not only did away with that behavior (embedded in the API itself) but expressly prohibits implementations from storing such system tags withinMetricID
instances. Instead, these system tags appear only in output returned from the metrics endpoints.As a result,
MetricsSettings
gains attributes for the app identifier and any global tags set via config so the code which creates the output has easy access to them.The new interface
SystemTagsManager
and its implementationSystemTagsManagerImpl
provide most of the logic for the new way of handling app and global tags.helidon-metrics-service-api
New interface and implemention for collecting and executing work to be done after a response has been sent (
PostRequestMetricsSupport
and its impl). This abstraction is useful in implementing the new spec requirement in which theREST.request
automatic metrics now must distinguish between successful and failed request processing. This abstraction provides a convenient way for callers to register post-request bits of code and have it run after the response is sent.We previously had a one-off, ad hoc implementation of this concept for extended key performance indicator metrics (which, similarly, needed to know whether request processing worked or not to update metrics correctly). With the new spec requirement for
REST.request
metrics to similarly differentiate, the abstraction and generalization became useful.helidon-metrics
(metrics/metrics
)HelidonConcurrentGauge
- Substitute locked access tolong
s rather than accesses toAtomicLong
s and somesynchronized
methods.HelidonGauge
- Some clean-up now that the spec was improved to require gauge parameterized types to extendNumber
.HelidonHistogram
- Implement new requiredsum
attribute.HelidonSimpleTimer
- Implement new requiredmin
' andmax
over the previous complete minute. Uses a read-write lock to control access to shared data internally. Also fixed a bug that failed to account for units in JSON output.HelidonTimer
- Implement new requiredelapsedTime
value.MetricImpl
- This superclass of all metric impl classes knows about the new way of handling system tags for output. Some other minor clean-up and efficiency improvements.MetricsSupport
(the metrics REST service) - Initializes the system tags manager from metrics settings (programmatically set or via config). Registers and executes the now-more-general post-request processing scheme for KPI metrics andREST.request
metrics. Added a slight optimization when responding to a request to/metrics
for metadata of a single metric.module-info
- Remove improperrequires
of MP config modules.HelidonSimpleTimerTest
- In addition to new feature tests, tests the bug fix about JSON output with units.helidon-microprofile-metrics
(microprofile/metrics
)MetricWorkItem
,BasicMetricWorkItem
, andSyntheticMetricWorkItem
- Abstraction interface and impls for metrics-related work done by metrics interceptors. Supports the new distinction we need to make between successful and unsuccessful processing for updating theREST.request
metrics.Renamed/repurposed
InterceptorSimplyTimedBase
toInterceptorSyntheticRestRequest
also due to the need to differentiate between successful and unsuccessful processing results in updating metrics.MetricProducer
- Removed code related to the@VendorDefined
qualifier.MetricsCdiExtension
- Removed rekated to the@VendorDefined
qualifier.In some cases the code deals with all metrics-related annotations; in others we exclude
@Gauge
; I added or renamed some constants to clarify the usage.I changed the display name and description of the automatic
REST.request
metrics to match what is in the spec.The spec now requires us to support metrics annotations via CDI stereotypes.
The simplification in the spec and supporting issue and PR related to app-provided producers let me remove a lot of producer-related code from the extension.
There are some metrics config settings, per spec, under
mp.metrics.xxx
while Helidon supports other Helidon-specific settings atmetrics.yyy
(in SE and MP). Some new code combines those in preparing the settings that determine the overall metrics system's behavior. (Bug fix)Due to the required support for stereotypes, various observers and related methods are now generalized to detect metrics annotations either directly on the annotated element or conveyed by a stereotype on the element.