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

Integrate micrometer metering #2133

Merged
merged 1 commit into from
Feb 11, 2021

Conversation

pravussum
Copy link
Contributor

@pravussum pravussum commented Jan 18, 2021

This is the initial draft of the micrometer integration to address #774.

I figured the best place to put it would be the already existing core.io.monitoring bundle. I'll move it otherwise.

Some notes:

  • the bundle registers some "default" metrics (basically all JVM related) and some openhab specific metrics (ThreadManager pool statistics, event counts, bundle states, thing states, rule run counts)
  • it uses the Micrometer global meter registry, a composite registry where other registries (e. g. the Prometheus one) can attach later to generate the actual metrics
  • I'm developing an IO addon, which attaches to the global meter registry and provides the actual metrics as REST endpoint (PR [io.metrics] initial contribution openhab-addons#9890)
  • other new Metrics could easily be added by registering with the global meter registry at their own convenience within their respective bundle

Some questions:

  • I had a hard time to get the Maven dependencies, bundle dependencies and Karaf features straight. Is this the right way to include a third party Maven dependency (non OSGI-ready) including its transitive dependencies? See https://community.openhab.org/t/3rd-party-maven-dependencies-in-openhab-core/113722 for a related forum post as well. I ended up using Karafs wrap feature instead of adding the dependency to the osgiify repo - although no one else seems to have used it that way.

/cc @kaikreuzer

@pravussum pravussum requested a review from a team as a code owner January 18, 2021 21:16
@wborn wborn added rebuild Triggers the Jenkins PR build and removed rebuild Triggers the Jenkins PR build labels Jan 18, 2021
@wborn
Copy link
Member

wborn commented Jan 18, 2021

I saw a Micrometer presentation once and ever since put it on my todo list to start integrating it into projects. So thanks for creating this PR. 👍

@wborn wborn added enhancement An enhancement or new feature of the Core work in progress A PR that is not yet ready to be merged labels Jan 19, 2021
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/3rd-party-maven-dependencies-in-openhab-core/113722/2

Signed-off-by: Robert Bach <openhab@mortalsilence.net>
@pravussum pravussum changed the title [WIP] Integrated micrometer metering #774 Integrated micrometer metering #774 Jan 23, 2021
@pravussum
Copy link
Contributor Author

I think this is in a state where it can be reviewed. Not much action to be seen without the addon PR, though.

@wborn wborn removed the work in progress A PR that is not yet ready to be merged label Jan 23, 2021
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/new-add-on-bundle-for-prometheus-health-metrics/48094/46

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks @pravussum, this looks awesome to me!
From my perspective, it's ready to merge.
Since I have never worked with micrometer so far, I'd be happy for a second review by @wborn or @cweitkamp, though.

@kaikreuzer kaikreuzer requested a review from wborn January 26, 2021 21:47
@kaikreuzer
Copy link
Member

@openhab/core-maintainers If you're not too familiar with it yourself and don't find the time for a review here, it is absolutely fine and I'd just go ahead and merge.

@cweitkamp
Copy link
Contributor

cweitkamp commented Feb 11, 2021

I am afraid I will not have time to test it now. Fell free to merge this PR. I will test it afterwards in my prod environment.

@kaikreuzer
Copy link
Member

Fair enough, let's merge then - thanks @pravussum, looking forward to your work on the add-on side!

@kaikreuzer kaikreuzer merged commit f061512 into openhab:main Feb 11, 2021
@kaikreuzer kaikreuzer added this to the 3.1 milestone Feb 11, 2021
@kaikreuzer kaikreuzer changed the title Integrated micrometer metering #774 Integrate micrometer metering Feb 11, 2021
@pravussum pravussum deleted the micrometer-integration branch February 11, 2021 20:26
@cweitkamp cweitkamp modified the milestone: 3.1 Mar 7, 2021
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/how-to-move-a-github-issue-to-the-right-repo/147489/1

splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 11, 2023
Signed-off-by: Robert Bach <openhab@mortalsilence.net>
GitOrigin-RevId: f061512
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core new contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants