Skip to content

Add content type to additional data spans #388

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.

Sign up for GitHub

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

Merged
shashank11p merged 5 commits into from
Oct 17, 2023
Merged

Add content type to additional data spans #388

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
50874ce
add conetnt type to additional data spans
shashank11p Sep 25, 2023
40de66d
add test
shashank11p Sep 25, 2023
e37fc3a
add content type to vertx additional-data spans
shashank11p Sep 25, 2023
d1c3803
get the method on startup
shashank11p Oct 4, 2023
798ab73
spotless apply
shashank11p Oct 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 31 additions & 6 deletions ...opentelemetry/javaagent/instrumentation/hypertrace/java/inputstream/InputStreamUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,16 @@
import io.opentelemetry.api.GlobalOpenTelemetry;
import io.opentelemetry.api.common.AttributeKey;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.SpanBuilder;
import io.opentelemetry.api.trace.Tracer;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.util.VirtualField;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.UnsupportedEncodingException;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.nio.charset.Charset;
import org.hypertrace.agent.core.instrumentation.HypertraceCallDepthThreadLocalMap;
import org.hypertrace.agent.core.instrumentation.HypertraceSemanticAttributes;
Expand All @@ -50,12 +53,34 @@ public static void addAttribute(Span span, AttributeKey<String> attributeKey, St
if (span.isRecording()) {
span.setAttribute(attributeKey, value);
} else {
TRACER
.spanBuilder(HypertraceSemanticAttributes.ADDITIONAL_DATA_SPAN_NAME)
.setParent(Context.root().with(span))
.setAttribute(attributeKey, value)
.startSpan()
.end();
SpanBuilder spanBuilder =
TRACER
.spanBuilder(HypertraceSemanticAttributes.ADDITIONAL_DATA_SPAN_NAME)
.setParent(Context.root().with(span))
Copy link
Copy Markdown

@ryanericson ryanericson Sep 25, 2023

Choose a reason for hiding this comment

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

Why is this Context.root() instead of Context.current()?

Copy link
Copy Markdown
Contributor Author

@shashank11p shashank11p Sep 25, 2023

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

@ryanericson ryanericson Sep 26, 2023

Choose a reason for hiding this comment

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

Which span has ended?

Copy link
Copy Markdown
Contributor Author

@shashank11p shashank11p Oct 4, 2023

Choose a reason for hiding this comment

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

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.

.setAttribute(attributeKey, value);

// Also add content type if present
if (span.getClass().getName().equals("io.opentelemetry.sdk.trace.SdkSpan")) {
try {
Method getAttribute =
Copy link
Copy Markdown
Contributor Author

@shashank11p shashank11p Sep 25, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

@ryanericson ryanericson Sep 25, 2023

Choose a reason for hiding this comment

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

Can we store content type in context along with parent span? Rather than retrieving with reflection from span.

Copy link
Copy Markdown
Contributor Author

@shashank11p shashank11p Sep 25, 2023

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

@ryanericson ryanericson Sep 25, 2023

Choose a reason for hiding this comment

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

What method is this instrumentation on?

Also how many framework instrumentation points do we support streaming bodies for?

Copy link
Copy Markdown
Contributor Author

@shashank11p shashank11p Sep 25, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

@ryanericson ryanericson Sep 26, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@shashank11p shashank11p Oct 4, 2023

Choose a reason for hiding this comment

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

Yes, added the method to static block during instantiation. Will be called only once.

span.getClass().getDeclaredMethod("getAttribute", AttributeKey.class);
getAttribute.setAccessible(true);
Object reqContentType =
getAttribute.invoke(span, AttributeKey.stringKey("http.request.header.content-type"));
if (reqContentType != null) {
spanBuilder.setAttribute("http.request.header.content-type", (String) reqContentType);
}
Object resContentType =
getAttribute.invoke(
span, AttributeKey.stringKey("http.response.header.content-type"));
if (resContentType != null) {
spanBuilder.setAttribute("http.response.header.content-type", (String) resContentType);
}
} catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException e) {
// ignore and continue
Comment thread
shashank11p marked this conversation as resolved.
}
}
spanBuilder.startSpan().end();
}
}

Expand Down
47 changes: 47 additions & 0 deletions ...ent/instrumentation/hypertrace/java/inputstream/InputStreamInstrumentationModuleTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,4 +111,51 @@ private void read(InputStream inputStream, Runnable read, String expected) {
SpanData spanData = trace.get(0);
Assertions.assertEquals(expected, spanData.getAttributes().get(ATTRIBUTE_KEY));
}

@Test
public void readAfterSpanEnd() {

InputStream inputStream = new ByteArrayInputStream(STR.getBytes());

Span span =
TEST_TRACER
.spanBuilder("test-span")
.setAttribute("http.request.header.content-type", "application/json")
.setAttribute("http.response.header.content-type", "application/xml")
.startSpan();

Runnable read =
() -> {
while (true) {
try {
if (inputStream.read(new byte[10], 0, 10) == -1) break;
span.end();
} catch (IOException e) {
e.printStackTrace();
}
;
}
};

BoundedByteArrayOutputStream buffer =
BoundedBuffersFactory.createStream(StandardCharsets.ISO_8859_1);
ContextAccessor.addToInputStreamContext(
inputStream, new SpanAndBuffer(span, buffer, ATTRIBUTE_KEY, StandardCharsets.ISO_8859_1));

read.run();

List<List<SpanData>> traces = TEST_WRITER.getTraces();
Assertions.assertEquals(1, traces.size());

List<SpanData> trace = traces.get(0);
Assertions.assertEquals(2, trace.size());
SpanData spanData = trace.get(1);
Assertions.assertEquals(STR, spanData.getAttributes().get(ATTRIBUTE_KEY));
Assertions.assertEquals(
"application/json",
spanData.getAttributes().get(AttributeKey.stringKey("http.request.header.content-type")));
Assertions.assertEquals(
"application/xml",
spanData.getAttributes().get(AttributeKey.stringKey("http.response.header.content-type")));
}
}
34 changes: 28 additions & 6 deletions ...opentelemetry/javaagent/instrumentation/hypertrace/vertx/ResponseBodyWrappingHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,15 @@
package io.opentelemetry.javaagent.instrumentation.hypertrace.vertx;

import io.opentelemetry.api.GlobalOpenTelemetry;
import io.opentelemetry.api.common.AttributeKey;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.SpanBuilder;
import io.opentelemetry.api.trace.Tracer;
import io.opentelemetry.context.Context;
import io.vertx.core.Handler;
import io.vertx.core.buffer.Buffer;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import org.hypertrace.agent.core.instrumentation.HypertraceSemanticAttributes;

public class ResponseBodyWrappingHandler implements Handler<Buffer> {
Expand All @@ -43,12 +47,30 @@ public void handle(Buffer event) {
if (span.isRecording()) {
span.setAttribute(HypertraceSemanticAttributes.HTTP_RESPONSE_BODY, responseBody);
} else {
tracer
.spanBuilder(HypertraceSemanticAttributes.ADDITIONAL_DATA_SPAN_NAME)
.setParent(Context.root().with(span))
.setAttribute(HypertraceSemanticAttributes.HTTP_RESPONSE_BODY, responseBody)
.startSpan()
.end();
SpanBuilder spanBuilder =
tracer
.spanBuilder(HypertraceSemanticAttributes.ADDITIONAL_DATA_SPAN_NAME)
.setParent(Context.root().with(span))
.setAttribute(HypertraceSemanticAttributes.HTTP_RESPONSE_BODY, responseBody);

// Also add content type if present
if (span.getClass().getName().equals("io.opentelemetry.sdk.trace.SdkSpan")) {
try {
Method getAttribute =
span.getClass().getDeclaredMethod("getAttribute", AttributeKey.class);
getAttribute.setAccessible(true);
Object resContentType =
getAttribute.invoke(
span, AttributeKey.stringKey("http.response.header.content-type"));
if (resContentType != null) {
spanBuilder.setAttribute("http.response.header.content-type", (String) resContentType);
}
} catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException e) {
// ignore and continue
}
}

spanBuilder.startSpan().end();
}

wrapped.handle(event);
Expand Down
5 changes: 5 additions & 0 deletions ...ing-common/src/testFixtures/java/org/hypertrace/agent/testing/AbstractHttpClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package org.hypertrace.agent.testing;

import io.opentelemetry.api.common.AttributeKey;
import io.opentelemetry.sdk.trace.data.SpanData;
import java.io.BufferedReader;
import java.io.IOException;
Expand Down Expand Up @@ -118,6 +119,10 @@ public void postJson_echo()
Assertions.assertEquals(2, traces.get(0).size());
SpanData responseBodySpan = traces.get(0).get(1);
assertBodies(clientSpan, responseBodySpan, body, body);
Assertions.assertNotNull(
responseBodySpan
.getAttributes()
.get(AttributeKey.stringKey("http.response.header.content-type")));
} else {
Assertions.assertEquals(1, traces.get(0).size());
assertRequestAndResponseBody(clientSpan, body, body);
Expand Down