-
Notifications
You must be signed in to change notification settings - Fork 1k
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
base: main
Are you sure you want to change the base?
Kotlin-specific Observation API #4823
Conversation
47f90a3
to
d100773
Compare
Hm. Looks like some unrelated tests are failing |
d100773
to
d9c279c
Compare
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.
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() }, |
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.
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.
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:Observation.observe
, but accepts a block of code instead of expecting a Java SupplierObservation.observeAndAwait
allows to observe a suspending block of code:Observation.observe
, but will accept a suspending block of codeopenScope().use { observationRegistry.asContextElement() }
it manages to create an instance ofKotlinObservationContextElement
with the new observation captured as "current"gh-4754