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
Add a Micrometer extension #11073
Add a Micrometer extension #11073
Conversation
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.
Overall it looks great, just a few minor comments
...icrometer/deployment/src/main/java/io/quarkus/micrometer/deployment/MicrometerProcessor.java
Show resolved
Hide resolved
integration-tests/micrometer-jmx/src/main/resources/application.properties
Outdated
Show resolved
Hide resolved
integration-tests/micrometer-mp-metrics/src/main/resources/application.properties
Show resolved
Hide resolved
extensions/micrometer/runtime/src/main/resources/META-INF/quarkus-extension.yaml
Outdated
Show resolved
Hide resolved
---- | ||
|
||
This command generates a Maven project, that imports the `micrometer` extension and the micrometer | ||
prometheus registry as dependencies. |
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.
Does this add the prometheus registry dependency automatically? I thought it would just be the micrometer extension only and the prometheus registry needs to be manually added?
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.
Not sure if you sure this comment @ebullient?
Other than the one comment on the guide, I think it's good from my side. Next week we should move it out of draft status to have a chance of making 1.8 release |
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.
I haven't looked at the code in detail but spotted a few issues.
Could you take care of them?
extensions/micrometer/runtime/src/main/resources/META-INF/quarkus-extension.yaml
Outdated
Show resolved
Hide resolved
CR1 will be released on September 2nd so if you plan to get this one, it needs to be merged by September 1st. |
Building now. Thanks, @gsmet! =) |
It will also need a rebase. |
Naturally. The documentation step failed with this:
I am pretty sure the dependencies for the integration test are correct, but I wonder if the Documentation CI step is missing something? |
Question: is there any native tests? I don't see any addition of a module to |
Right. There are native tests (nothing fancy) that I was running in my own repo.. I forgot about the bucket definitions in ci-actions.yml. Should I add them to one of the existing categories? Make a new one? |
I would add them together with metrics and add a 5 minutes more to the timeout. Also better rebase to get the latest CI fixes. |
Rebased. smallrye-metrics doesn't have dedicated integration tests (micrometer does to account for different backends). I added two of them to one of the misc buckets. Can change it if you want. |
There are two classloading problems in the non-micrometer-integration-tests running in native mode that were touched to use the micrometer extension instead of mp-metrics. I'm working on it. |
There are issues in the doc:
Please make a visual pass locally on the rendered output of the documentation when you're adding a significant piece of doc. |
Yep. I saw that. I'm trying to sort that out. I have something locally (I needed to sleep ;) ) FWIW, I did check the docs in my other repo, but the behavior is a bit different when the contents are nested in a larger doc, and that's what is tripping it up (nested table). If I can't get it sorted, I'll ditch the table and go back to a list. |
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.
Minor comment on the doc related to what dependencies get added
---- | ||
|
||
This command generates a Maven project, that imports the `micrometer` extension and the micrometer | ||
prometheus registry as dependencies. |
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.
Not sure if you sure this comment @ebullient?
Enable MP Metrics if API is present by default
All set, I think. =) |
OK, let's get this one merged. Thanks @ebullient ! |
Introduce micrometer extension to provide support for micrometer metrics. Acts as an alternative to the smallrye metrics extension. Each extension provides a MetricsCapabilityBuildItem, which is a SimpleBuildItem; a project's build will fail with a message if both are enabled.