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

Add "createNotStarted" method that lazily creates context #3401

Merged

Conversation

ttddyy
Copy link
Contributor

@ttddyy ttddyy commented Sep 9, 2022

Add another variant of observation method to delay the observation context creation. When the registry is a no-op, it fast returns the no-op observation and skips calling the supplier. This avoids the creation of the observation context.

Ideally, the supplier should be pushed down to more method calls to further delay the context creation, but at least this gives an API that minimize the creation of context.

* @param registry observation registry
* @return observation
*/
default <T extends Observation.Context> Observation createNotStarted(
Copy link
Member

Choose a reason for hiding this comment

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

Can we also add a null check since the registry is @Nullable? See:

if (registry == null || registry.isNoop()
|| !registry.observationConfig().isObservationEnabled(name, context)) {
return NOOP;
}

Also, should not we do this in the Observation too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

registry is non-null on this method, so don't need null check here.

I'll add documentation that this no-op registry check is a quick check and further decision(ObservationPredicate) would happen after context is created.

Copy link
Member

Choose a reason for hiding this comment

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

I'll go to an eye doctor. :D

* @param registry observation registry
* @return observation
*/
default <T extends Observation.Context> Observation createNotStarted(
Copy link
Member

Choose a reason for hiding this comment

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

I think we should call these methods createNotStarted but I'm not sure we all agreed on that.

I think either we call all of them createNotStarted or observation.

Add another variant of "observation" method to delay the observation
context creation.  When the registry is a no-op, it fast returns the
no-op observation and skips calling the supplier.  This avoids the
creation of the observation context.
@marcingrzejszczak marcingrzejszczak added this to the 1.10.0-M5 milestone Sep 12, 2022
@marcingrzejszczak marcingrzejszczak merged commit 9c5b9f2 into micrometer-metrics:main Sep 12, 2022
@ttddyy ttddyy deleted the create-not-started branch September 12, 2022 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants