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 all 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
54 changes: 48 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 @@ -42,6 +45,23 @@ private InputStreamUtils() {}
private static final Tracer TRACER =
GlobalOpenTelemetry.get().getTracer("org.hypertrace.java.inputstream");

private static Method getAttribute = null;

static {
try {
getAttribute =
Class.forName("io.opentelemetry.sdk.trace.SdkSpan")
.getDeclaredMethod("getAttribute", AttributeKey.class);
} catch (NoSuchMethodException e) {
log.error("getAttribute method not found in SdkSpan class", e);
} catch (ClassNotFoundException e) {
log.error("SdkSpan class not found", e);
}
if (getAttribute != null) {
getAttribute.setAccessible(true);
}
}

/**
* Adds an attribute to span. If the span is ended it adds the attributed to a newly created
* child.
Expand All @@ -50,12 +70,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 (getAttribute != null
&& span.getClass().getName().equals("io.opentelemetry.sdk.trace.SdkSpan")) {
try {
Object reqContentType =
getAttribute.invoke(
span, HypertraceSemanticAttributes.HTTP_REQUEST_HEADER_CONTENT_TYPE);
if (reqContentType != null) {
spanBuilder.setAttribute("http.request.header.content-type", (String) reqContentType);
}
Object resContentType =
getAttribute.invoke(
span, HypertraceSemanticAttributes.HTTP_RESPONSE_HEADER_CONTENT_TYPE);
if (resContentType != null) {
spanBuilder.setAttribute("http.response.header.content-type", (String) resContentType);
}
} catch (IllegalAccessException | InvocationTargetException e) {
// ignore and continue
log.debug("Could not invoke getAttribute on SdkSpan", e);
}
}
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")));
}
}
54 changes: 48 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,18 +17,43 @@
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;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class ResponseBodyWrappingHandler implements Handler<Buffer> {

private static final Tracer tracer =
GlobalOpenTelemetry.getTracer("io.opentelemetry.javaagent.vertx-core-3.0");

private static final Logger log = LoggerFactory.getLogger(ResponseBodyWrappingHandler.class);

private static Method getAttribute = null;

static {
try {
getAttribute =
Class.forName("io.opentelemetry.sdk.trace.SdkSpan")
.getDeclaredMethod("getAttribute", AttributeKey.class);
} catch (NoSuchMethodException e) {
log.error("getAttribute method not found in SdkSpan class", e);
} catch (ClassNotFoundException e) {
log.error("SdkSpan class not found", e);
}
if (getAttribute != null) {
getAttribute.setAccessible(true);
}
}

private final Handler<Buffer> wrapped;
private final Span span;

Expand All @@ -43,12 +68,29 @@ 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 (getAttribute != null
&& span.getClass().getName().equals("io.opentelemetry.sdk.trace.SdkSpan")) {
try {
Object resContentType =
getAttribute.invoke(
span, HypertraceSemanticAttributes.HTTP_RESPONSE_HEADER_CONTENT_TYPE);
if (resContentType != null) {
spanBuilder.setAttribute("http.response.header.content-type", (String) resContentType);
}
} catch (IllegalAccessException | InvocationTargetException e) {
// ignore and continue
log.debug("Could not invoke getAttribute on SdkSpan", e);
}
}

spanBuilder.startSpan().end();
}

wrapped.handle(event);
Expand Down
6 changes: 6 additions & 0 deletions ...src/main/java/org/hypertrace/agent/core/instrumentation/HypertraceSemanticAttributes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ public static AttributeKey<String> httpResponseHeader(String header) {
public static final AttributeKey<String> HTTP_REQUEST_SESSION_ID =
AttributeKey.stringKey("http.request.session_id");

public static final AttributeKey<String> HTTP_REQUEST_HEADER_CONTENT_TYPE =
AttributeKey.stringKey("http.request.header.content-type");

public static final AttributeKey<String> HTTP_RESPONSE_HEADER_CONTENT_TYPE =
AttributeKey.stringKey("http.response.header.content-type");

public static final AttributeKey<String> RPC_REQUEST_BODY =
AttributeKey.stringKey("rpc.request.body");
public static final AttributeKey<String> RPC_RESPONSE_BODY =
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