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

Make more explicit that recording a metric is safe to call #5365

Merged

Conversation

cbrachem
Copy link
Contributor

@cbrachem cbrachem commented Aug 2, 2024

I've added a TIP block to the Registry's docs to make it more explicit what happens when operations on Meters like incrementCount are called, which will make it clear that such operations are safe to call from regular code.

This PR is the result of a discussion in Slack.

Of course, feel free to challenge the general usefulness of the added paragraph, its accuracy and wording.

Copy link
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request. This is useful info to add to the docs for sure. I left some review.

@@ -3,6 +3,8 @@

A `Meter` is the interface for collecting a set of measurements (which we individually call metrics) about your application. Meters in Micrometer are created from and held in a `MeterRegistry`. Each supported monitoring system has an implementation of `MeterRegistry`. How a registry is created varies for each implementation.

TIP: Recoding a measurement for a `Meter` is expected to be a simple math/tallying operation that will not consume any significant amount of computing time or throw exceptions. Transferring metrics to an external monitoring system is a scheduled operation on a separate thread and will not interfere with meter recordings.
Copy link
Member

Choose a reason for hiding this comment

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

I would simplify the first part to something along the lines "recording a measurement for a Meter is expected to be a relatively cheap operation and it should not throw any exception."
The second sentence is about publishing which is a bit implementation dependent. Perhaps I would word it as something like "if the registry supports publishing metrics to a metrics backend, this should be done on a separate thread from recording metrics"

There's also a bit of a question of whether both these belong in this page (Registry) of the docs. Maybe we should move the Meters page to before the Registry page in the table of contents and have the first part about recording measurements there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would simplify the first part to something along the lines "recording a measurement for a Meter is expected to be a relatively cheap operation and it should not throw any exception."
The second sentence is about publishing which is a bit implementation dependent. Perhaps I would word it as something like "if the registry supports publishing metrics to a metrics backend, this should be done on a separate thread from recording metrics"

Sounds good, I will rephrase according to your suggestion.

There's also a bit of a question of whether both these belong in this page (Registry) of the docs. Maybe we should move the Meters page to before the Registry page in the table of contents and have the first part about recording measurements there?

Interesting. That might be nice. Let me try and move things around a bit and then we can see how it looks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shakuzen Is this what you had in mind?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that looks good to me. I left a minor review comment.

@shakuzen shakuzen added the doc-update A documentation update label Aug 7, 2024
@shakuzen shakuzen added this to the 1.12.9 milestone Aug 7, 2024
Copy link
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

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

Thank you for the documentation improvement.

@shakuzen shakuzen changed the base branch from main to 1.12.x August 7, 2024 01:55
@shakuzen shakuzen force-pushed the docs-meter-safe-to-call branch from 6255a0e to 1efffe9 Compare August 7, 2024 01:56
@shakuzen
Copy link
Member

shakuzen commented Aug 7, 2024

I've rebased the pull request on the 1.12.x branch so we can add apply this change which is applicable to the oldest version of the docs maintained here and merge forward.

@shakuzen shakuzen merged commit a299d10 into micrometer-metrics:1.12.x Aug 7, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-update A documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants