Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Instrumentation API #165
Instrumentation API #165
Changes from 3 commits
b034ba2
34408c8
1338129
80bbaa3
a233f47
8335c9a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I may have misunderstood something here.... however, based on my reading of this OTEP, I don't understand how this
Extractor
pattern results in less work re: semantic conventions for library authors? How can I build anExtractor
class thatmap[s] from a request or response object into attributes
without knowing the actual semantic conventions involved?Is there perhaps a concise snippet of code you could share that would illustrate how this might be used by a third-party library to instrument their library for OpenTelemetry? I found a Spring integration PR that uses this framework in the
opentelemetry-java-instrumentation
repo, but that particular example doesn't really clear up much for me.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.
Typo:
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.
This seems as though it would be difficult to things that do not have a clear request/response cycle. If, for instance, one were building instrumentation for something like
ActionView
in Rails and wished to create a span when a template were rendered into the HTML response, we'd need to then construct some kind of artificialrequest
abstraction to pass to this method.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.
The instrumentation API is meant to make standard telemetry instrumentation easier, I could try to write that but a bit unsure the best way to phrase it. Internal implementation details like a view may be best with the OpenTelemetry API especially if it doesn't semantic conventions - the main reason to use a mapping of request to attributes is to enforce the conventions. Internal instrumentation also doesn't need to worry about propagation, nesting, etc that this API tries to tackle
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.
I mentioned that specific example because I've been working on optional auto-instrumentation for the
opentelemetry-ruby
project that instruments parts of Rails like that automatically, and I think other frameworks might fall into that category too. I agree that end-users need not use this Instrumentation API, but people like me (writing auto-instrumentation for others) would. That's the kind of thing I had in mind, if that helps clarify why I mentioned it?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.
If we are to make instrumentation easier for library authors, then we should consider if there is anything else we could to to reduce manual
context
management. For example, is there a way to build thisInstrumentation API
such that "dangling spans" (those that the library author forgot toend
) are automatically cleaned up?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.
If context could be handled by magic without a javaagent then life would be great :) No ideas on how to improve the general pattern of context management, though at least having 1 method that returns a
Context
and another that is documented to accept it is still a win over OpenTelemetry API I think - I've seen code that creates a context (possibly not parenting correctly) to create a span, then passes in an arbitraryContext.current()
when ending instead of the instrumentation context.For dangling spans, I think this is quite language specific. In Java, we can implement a "GC hook" as a span processor to catch spans that get GC'd without
end
being called. Main reason for not having implemented this yet is not knowing what to do with those spans as it probably needs some specification (Zipkin exports the span context (not the span, it's already collected) with withflush=true
attribute).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.
True. 😄
I wonder if this is an opportunity to improve the next iteration of the Context / Span APIs themselves?
This is probably true, and I think we can probably just ignore my comments in this section because of that. Although, maybe mentioning here that languages implementing the Instrumentation API "SHOULD provide an automatic cleanup mechanism for authors", but the value of that may be debatable still.
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.
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.
Required by whom? The instrumentation library author or the SDK?
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.
The Instrumentation API, at least in this case by using abstract methods in Java. Indeed this may not translate to other languages but even without the strict requirement, it could be more discoverable and ensure filling in attributes compared to
setAttribute
I thinkThere 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.
Sorry - let me rephrase. Are we saying here that we expect instrumentation authors to implement all of the abstract methods, even if they don't use them? Or that we expect the people who actually build the instrumentation API itself to implement no-op implementations of all attributes?
I do like the focus on explicitness, in general, but whether or not I personally like this part of the idea depends on who we expect to be implementing these methods, and when. 😄
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.
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.
I wonder if we should pass any caught error to the listener too:
For example, if you'd like to implement tracing (or something similar to it) in a
RequestListener
you'd need that error to callspan.recordException()
. Everything aREQUEST
orRESPONSE
contains is already translated toAttributes
, the error is the only thing missing.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.
Should it be "response"?
Or maybe request and response, now that I think of it.
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.
I respectfully disagree. 😄
As a career Rubyist, this doesn't feel like something I could easily or idiomatically adapt to in the Ruby world. That is not to say it is a good proposal or bad proposal, but rather to say that this does feel rather Java-specific to me, at least.