Add content type to additional data spans#388
Conversation
|
Have manually tested this change as well. |
| // Also add content type if present | ||
| if (span.getClass().getName().equals("io.opentelemetry.sdk.trace.SdkSpan")) { | ||
| try { | ||
| Method getAttribute = |
There was a problem hiding this comment.
Needed to use reflection because this part of code will be running in the application class loader. And that will not have the sdk libraries which has this SdkSpan class.
There was a problem hiding this comment.
Can we store content type in context along with parent span? Rather than retrieving with reflection from span.
There was a problem hiding this comment.
We can but we will need that change in all the framework instrumentations that we want to support. This was a one place change for this for all parent frameworks
There was a problem hiding this comment.
What method is this instrumentation on?
Also how many framework instrumentation points do we support streaming bodies for?
There was a problem hiding this comment.
What method is this instrumentation on?
This is instrumented on the read method in java.io.InputStream
Also how many framework instrumentation points do we support streaming bodies for?
We have 4 client instrumentations and all support streaming.
There was a problem hiding this comment.
Ok, my concern is with overhead, will let you take call on which approach to go. If we go with reflection, should we at least cache the Method object because that wouldn't change, will it? If that's not an expensive operation then nvm.
There was a problem hiding this comment.
Yes, added the method to static block during instantiation. Will be called only once.
| SpanBuilder spanBuilder = | ||
| TRACER | ||
| .spanBuilder(HypertraceSemanticAttributes.ADDITIONAL_DATA_SPAN_NAME) | ||
| .setParent(Context.root().with(span)) |
There was a problem hiding this comment.
Why is this Context.root() instead of Context.current()?
There was a problem hiding this comment.
Because if the span has ended, then the current may not actually be the parent of this additional-data.
It could be inside the scope of another span, but we want this additional-data with response body to be present under the span which we are passing down from the VirtualStore
There was a problem hiding this comment.
So if we had a root span -> parent span (which has the inputstream).
Now, the parent span is ended, but the inputstream.read() is not yet called, but the inputstream is passed down, and another span is created, and then the input stream is read. Then the current span where input stream is read is not the actual span under which we want to create the additional data span. So that may the reason. We would want it like:
root span:
parent span(ended):
additional span:
new span(calls inputstream.read()):
I wasn't involved when this code was written, so I'm also trying to think and justify this.
...io/opentelemetry/javaagent/instrumentation/hypertrace/java/inputstream/InputStreamUtils.java
Show resolved
Hide resolved
| SpanBuilder spanBuilder = | ||
| TRACER | ||
| .spanBuilder(HypertraceSemanticAttributes.ADDITIONAL_DATA_SPAN_NAME) | ||
| .setParent(Context.root().with(span)) |
| // Also add content type if present | ||
| if (span.getClass().getName().equals("io.opentelemetry.sdk.trace.SdkSpan")) { | ||
| try { | ||
| Method getAttribute = |
There was a problem hiding this comment.
Ok, my concern is with overhead, will let you take call on which approach to go. If we go with reflection, should we at least cache the Method object because that wouldn't change, will it? If that's not an expensive operation then nvm.
In case of capturing request/response bodies in additional data spans, we only have the body. Also adding the content type header in those spans along with body.