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

Kotlin-specific Observation API #4823

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ilya40umov
Copy link

@ilya40umov ilya40umov commented Mar 2, 2024

This PR introduces a couple of extension functions on Observation to help using it from Kotlin code.
It's an alternative version of the following PR: #4772, which would introduce the same API, but on ObservationRegistry-level.

  • Observation.observeAndGet allows to observe a non-suspending block of code:
    • this method is analogous to Observation.observe, but accepts a block of code instead of expecting a Java Supplier
    • this method will work with both nullable and non-nullable generic type (while usage of Supplier makes Kotlin compiler think the result of the nested block is always nullable)
  • Observation.observeAndAwait allows to observe a suspending block of code:
    • this method is again similar to Observation.observe, but will accept a suspending block of code
    • by using openScope().use { observationRegistry.asContextElement() } it manages to create an instance of KotlinObservationContextElement with the new observation captured as "current"

gh-4754

@ilya40umov
Copy link
Author

Hm. Looks like some unrelated tests are failing MongoMetricsCommandListenerTest > shouldCreateFailedCommandMetricWithCustomSettings() . 🤔

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, and sorry it took so long to get reviewed. The PR looks reasonable but there's one issue I think we need to figure out what, if anything, we can/should do. I'm raising it with the team.

start()
return try {
withContext(
openScope().use { observationRegistry.asContextElement() },
Copy link
Member

Choose a reason for hiding this comment

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

A consideration here is that this implementation depends on the context-propagation library, while Observation and ObservationRegistry don't directly - they optionally depend on it. Since we added the extension function asContextElement() to ObservationRegistry, that gives precedent to this kind of thing, but I'm not sure we should do this. That said, I don't know what a good alternative is. Basically, the issue is that this would fail at runtime if the user didn't have context-propagation on their classpath, which could happen if say they only care about producing metrics from Observation and not tracing. It feels bad to have a class that otherwise can be used without a library except an API (extension function) that will fail without a specific dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants