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

Metrics 4.0 implementation #3847

Merged
merged 17 commits into from
Feb 1, 2022
Merged

Metrics 4.0 implementation #3847

merged 17 commits into from
Feb 1, 2022

Conversation

tjquinno
Copy link
Member

@tjquinno tjquinno commented Jan 27, 2022

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

  1. 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.

  2. 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.

  3. 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.

  4. In several classes, synchronized methods and/or AtomicXXXs 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.

  5. 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.

  6. 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.

  7. 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.

  8. 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)

  1. 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.

    AbstractRegistrystill defines the API used internally by Helidon metrics, but now it largely delegates toMetricStore` for operations which update or query the data structures.

    As part of this, synchronization in MetricStore does not use the synchronized keyword on methods but rather uses a ReadWriteLock 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.

  2. The MP metrics 3.0 spec added several new methods to MetricRegistry which are reflected in AbstractRegistry.
    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.

  3. Although MetricSettings predates this PR, it gains two new attributes. A long-standing issue with MP metrics was that creating a new MetricID 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 within MetricID 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 implementation SystemTagsManagerImpl provide most of the logic for the new way of handling app and global tags.

helidon-metrics-service-api

  1. 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 the REST.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)

  1. HelidonConcurrentGauge - Substitute locked access to longs rather than accesses to AtomicLongs and some synchronized methods.

  2. HelidonGauge - Some clean-up now that the spec was improved to require gauge parameterized types to extend Number.

  3. HelidonHistogram - Implement new required sum attribute.

  4. HelidonSimpleTimer - Implement new required min' and max 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.

  5. HelidonTimer - Implement new required elapsedTime value.

  6. 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.

  7. 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 and REST.request metrics. Added a slight optimization when responding to a request to /metrics for metadata of a single metric.

  8. module-info - Remove improper requires of MP config modules.

  9. HelidonSimpleTimerTest - In addition to new feature tests, tests the bug fix about JSON output with units.

helidon-microprofile-metrics (microprofile/metrics)

  1. MetricWorkItem, BasicMetricWorkItem, and SyntheticMetricWorkItem - 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 the REST.request metrics.

  2. Renamed/repurposed InterceptorSimplyTimedBase to InterceptorSyntheticRestRequest also due to the need to differentiate between successful and unsuccessful processing results in updating metrics.

  3. MetricProducer - Removed code related to the @VendorDefined qualifier.

  4. 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 at metrics.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.

@tjquinno tjquinno added this to the 3.0.0 milestone Jan 27, 2022
@tjquinno tjquinno self-assigned this Jan 27, 2022
… Add dep on config yaml parser for examples (no longer dragged in automatically)
…e interceptor runs, to avoid race condition; add test
@tjquinno tjquinno marked this pull request as draft January 30, 2022 16:21
@tjquinno
Copy link
Member Author

Converted to draft because of an intermittent failure in the TCK and in a microprofile/metrics unit test. Continued reviews encouraged but pointless to approve yet.

@tjquinno tjquinno marked this pull request as ready for review January 31, 2022 18:18
Copy link
Member

@tomas-langer tomas-langer left a 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).

@tjquinno tjquinno requested a review from ljamen January 31, 2022 21:07
@tjquinno tjquinno merged commit 303112e into helidon-io:master Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metrics 4.0
4 participants