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

Redesign MeterRegistry usage in reactor #2253

Closed
ericbottard opened this issue Jul 6, 2020 · 0 comments · Fixed by #2265
Closed

Redesign MeterRegistry usage in reactor #2253

ericbottard opened this issue Jul 6, 2020 · 0 comments · Fixed by #2265
Assignees
Labels
type/enhancement A general enhancement
Milestone

Comments

@ericbottard
Copy link
Contributor

The current approach in reactor is that FluxMetrics(Fuseable) (and other similar classes) accept a reference to a micrometer MeterRegistry, but in practice always use Metrics.globalRegistry from micrometer (to avoid exposing a micrometer class in the Flux API that would cause a class loading exception if micrometer is not on the classpath).

This is problematic in two ways:

  • tests don't really test real world usage of the API, and when trying to use an explicit registry, sometimes pollute it or on the contrary clean up too much. See Fix MonoMetricsFuseableTest and SchedulersMetricsTest #2246
  • this doesn't give much flexibility to users, forcing the use of the one and only Metrics.globalRegistry when using reactor, which in turn often delegates to many registries.

The proposed solution is to introduce a way to configure the "registry to use with reactor", which by default could be Metrics.globalRegistry to remain backwards compatible. Tests could thus also pass in a throwaway registry per-test. This configuration option needs to be done in a way that doesn't trigger classloading of micrometer when not needed (and not present)

@reactorbot reactorbot added the ❓need-triage This issue needs triage, hasn't been looked at by a team member yet label Jul 6, 2020
@simonbasle simonbasle added status/need-design This needs more in depth design work and removed ❓need-triage This issue needs triage, hasn't been looked at by a team member yet labels Jul 6, 2020
@ericbottard ericbottard self-assigned this Jul 9, 2020
@simonbasle simonbasle added type/enhancement A general enhancement and removed status/need-design This needs more in depth design work labels Jul 16, 2020
@simonbasle simonbasle added this to the 3.4.0-M2 milestone Jul 16, 2020
@simonbasle simonbasle linked a pull request Jul 16, 2020 that will close this issue
ericbottard added a commit that referenced this issue Jul 16, 2020
This commit revisits the use of MicroMeter in project reactor by not
solely relying on MicroMeter's glogbalRegistry, but instead offering
a way to configure which registry to use prior to calling metrics().
The default value is still the globalRegistry, thus being fully backwards
compatible.
simonbasle pushed a commit that referenced this issue Aug 6, 2020
- `MonoMetrics` also uses the `Metrics`-level registry
- the refdoc uses `[name].` pattern for meter names everywhere
aneveu pushed a commit that referenced this issue Aug 7, 2020
aneveu pushed a commit that referenced this issue Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants