Skip to content

feat: extract micrometer support into its own module #577

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

Merged
merged 2 commits into from
Oct 5, 2021

Conversation

metacosm
Copy link
Collaborator

@metacosm metacosm commented Oct 4, 2021

Fixes #576. This allows getting rid of mandatory micrometer dependency
if users are not interested in having metrics.

@csviri
Copy link
Collaborator

csviri commented Oct 4, 2021

Made some comments on the original issue

@csviri csviri requested a review from lburgazzoli October 4, 2021 09:47
@csviri
Copy link
Collaborator

csviri commented Oct 4, 2021

I'm not against this, it makes sense. Just wonder if has that much added benefit, while we introduce an additional module. But again I'm not against this.

@lburgazzoli
Copy link
Collaborator

lburgazzoli commented Oct 4, 2021

I think the split makes sense however I think we should reason a little bit more about how metrics related bits are added into the code as example for what concerns DefaultEventHandler we don't to a great job:

https://github.com/java-operator-sdk/java-operator-sdk/blob/4d36922c90c49767a355c44006fc5586fe91a967/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java#L35-L44
https://github.com/java-operator-sdk/java-operator-sdk/blob/4d36922c90c49767a355c44006fc5586fe91a967/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java#L35-L46

In the code above we are saying:

  1. DefaultEventHandler is one of the possible implementation of EventHandler
  2. DefaultEventHandler has a static field that is used to configure metrics
  3. well, having the EventHandler interface in not very useful as we rely on a static field on DefaultEventHandler to set-up metrics.

So I wonder if instead, if metrics are enabled, then we should instantiate a InstrumentedEventHandler maybe provided by the micrometer support which IMHO, would make the addition of this new module even more meaningful (the InstrumentedEventHandler can be eventually loaded using the ServiceLoader mechanism, Quarkus could optimize things by pre-loading and configuring micrometer at build time)

@csviri
Copy link
Collaborator

csviri commented Oct 4, 2021

Definitely, will take a look on that also soon. Maybe we can discuss that under a new issue, and we can discuss the design there.

What I don't like on micrometer core that is has bunch of dependencies. That is a good argument to not have it on the classpath by default.
On the other hand it's de facto industry standard, and every production system needs to have metrics monitored. So eventually everybody will use this module. And more importantly they should.

So if you insist to merge this I'm fine. Just let's think in the future if it really has the benefit, since we have to then create additional abstractions for this in core.

@metacosm
Copy link
Collaborator Author

metacosm commented Oct 4, 2021

@csviri @lburgazzoli I changed how we deal with EventMonitor, hopefully it makes more sense now… I can't really do the full clean-up I'd like to until v2, though… Let me know what you think!

@csviri
Copy link
Collaborator

csviri commented Oct 4, 2021

LGTM

Fixes #576. This allows getting rid of mandatory micrometer dependency
if users are not interested in having metrics.
The goal is to not rely on DefaultEventHandler eventually. EventMonitor
was kept on DefaultEventHandler for backwards compatibility reason but
this should be moved to its own package along with the Metrics class
for v2
@metacosm metacosm force-pushed the extract-micrometer branch from dfe8474 to d821610 Compare October 5, 2021 09:05
@metacosm metacosm merged commit b54e610 into master Oct 5, 2021
@metacosm metacosm deleted the extract-micrometer branch October 5, 2021 09:12
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.

Provide a no-dependency Metrics class implementation
3 participants