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

Global observation registry #5700

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cfredri4
Copy link

This change adds a static ObservationRegistry.global similar to the Metrics.globalRegistry. For the same reasons, for use in places where dependency injection is not desired.

This change adds a static ObservationRegistry.global similar to the Metrics.globalRegistry. For the same reasons, for use in places where dependency injection is not desired.
@jonatan-ivanov
Copy link
Member

If you ask me, I'm not a big fan of the global MeterRegistry. In case of the ObservationRegistry, since

  • chances are much higher that you will end up with a multiple registries if libraries start using the global instead of letting the user inject one
  • it has components that were designed to be injected (ObservationConvention)
  • it has more config options and easier to misconfigure something

I think we should not do this.
As far as I remember we discussed the possibility of creating a global but we decided not to.

Also, could you please mention examples where dependency injection is not desired?

@jonatan-ivanov jonatan-ivanov added the waiting for feedback We need additional information before we can continue label Nov 22, 2024
@cfredri4
Copy link
Author

All perfectly valid objections.

The main reason for why dependency injection is not always desired is overhead. It is trivial for any application or library to register metrics (just Metrics.xxx()), but having to inject an observation registry everywhere observation is desired makes it less trivial. Especially for classes which are not managed by Spring.

My guess was that this had been considered in the past but wanted to raise this in case it was somehow missed. 😅

@jonatan-ivanov
Copy link
Member

What kind of "overhead" are you referring to? Is it performance, or possibly more code because you need an extra ctor param or setter?

It is trivial for any application or library to register metrics (just Metrics.xxx()), but having to inject an observation registry everywhere observation is desired makes it less trivial.

It is also easy to make mistakes and get into trouble with configuring things or the classloader. It is also very hard to test.

Please notice that:

  • In order to use dependency injection, you don't need a framework (like Spring)
  • You only need a ctor parameter (or setter)
  • You can always use a global in your code, you "only" need a public static final field (and probably a public wrapper class)

I still don't think it is a good idea to open this can of worms up and possibly encourage libraries to instrument themselves using a global registry instead of letting the users injecting one (in the past, users bumped into issues using the global meter registry).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for feedback We need additional information before we can continue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants