Skip to content

Otel optimizations I #47774

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
merged 1 commit into from
May 9, 2025
Merged

Otel optimizations I #47774

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 15 additions & 12 deletions ...entelemetry/runtime/src/main/java/io/quarkus/opentelemetry/runtime/OpenTelemetryUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;

import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.SpanContext;
Expand All @@ -19,7 +17,6 @@ public final class OpenTelemetryUtil {
public static final String SPAN_ID = "spanId";
public static final String SAMPLED = "sampled";
public static final String PARENT_ID = "parentId";
private static final Set<String> SPAN_DATA_KEYS = Set.of(TRACE_ID, SPAN_ID, SAMPLED, PARENT_ID);

private OpenTelemetryUtil() {
}
Expand Down Expand Up @@ -53,22 +50,28 @@ public static Map<String, String> convertKeyValueListToMap(List<String> headers)

/**
* Sets MDC data by using the current span from the context.
* <p>
* This method is in the hot path and was optimized to not use getSpanData()
*
* @param context opentelemetry context
* @param vertxContext vertx context
*/
public static void setMDCData(Context context, io.vertx.core.Context vertxContext) {
setMDCData(getSpanData(context), vertxContext);
}

public static void setMDCData(Map<String, String> spanData, io.vertx.core.Context vertxContext) {
if (spanData == null) {
if (context == null) {
return;
}

for (Entry<String, String> entry : spanData.entrySet()) {
if (SPAN_DATA_KEYS.contains(entry.getKey())) {
VertxMDC.INSTANCE.put(entry.getKey(), entry.getValue(), vertxContext);
Span span = Span.fromContextOrNull(context);
if (span != null) {
SpanContext spanContext = span.getSpanContext();
VertxMDC.INSTANCE.put(SPAN_ID, spanContext.getSpanId(), vertxContext);
VertxMDC.INSTANCE.put(TRACE_ID, spanContext.getTraceId(), vertxContext);
VertxMDC.INSTANCE.put(SAMPLED, Boolean.toString(spanContext.isSampled()), vertxContext);
if (span instanceof ReadableSpan) {
SpanContext parentSpanContext = ((ReadableSpan) span).getParentSpanContext();
if (parentSpanContext != null && parentSpanContext.isValid()) {
VertxMDC.INSTANCE.put(PARENT_ID, parentSpanContext.getSpanId(), vertxContext);
}
}
}
}
Expand All @@ -83,7 +86,7 @@ public static Map<String, String> getSpanData(Context context) {
return Collections.emptyMap();
}
Span span = Span.fromContextOrNull(context);
Map<String, String> spanData = new HashMap<>();
Map<String, String> spanData = new HashMap<>(4, 1f);
if (span != null) {
Copy link
Member

@gsmet gsmet May 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I wouldn't add these comments if you weren't already into micro-optimization mode but:

  • if span can be null quite often, you could also return Collections.emptyMap() in this case
  • I also wonder if it might be worth using Map.of and specializing the cases a bit more

I was thinking of something like:

    public static Map<String, String> getSpanData(Context context) {
        if (context == null) {
            return Collections.emptyMap();
        }

        Span span = Span.fromContextOrNull(context);
        if (span == null) {
            return Collections.emptyMap();
        }

        SpanContext spanContext = span.getSpanContext();
        if (span instanceof ReadableSpan readableSpan
                && readableSpan.getParentSpanContext() != null
                && readableSpan.getParentSpanContext().isValid()) {
            return Map.of(
                    SPAN_ID, spanContext.getSpanId(),
                    TRACE_ID, spanContext.getTraceId(),
                    SAMPLED, Boolean.toString(spanContext.isSampled()),
                    PARENT_ID, readableSpan.getParentSpanContext().getSpanId()
            );
        }

        return Map.of(
                SPAN_ID, spanContext.getSpanId(),
                TRACE_ID, spanContext.getTraceId(),
                SAMPLED, Boolean.toString(spanContext.isSampled())
        );
    }

It might not be worth the additional trouble though so I will let you be the judge of if. I don't expect this code to change often so I think the duplication is not that big of a problem if it performs faster.

Also the second part only works if we expect the Map to be immutable, obviously :).

Copy link
Contributor Author

@brunobat brunobat May 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Immutability is why I didn't think about the Map.of()... I'll give it a try.
Please mind the execution path using the HashMap is now only used for logging.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, if it doesn't look like a good idea, just don't bother :).
The first part about not creating the Map if null might be a good idea though.

I only added a comment because it was already micro optimization but at some point you have to decide that things are good enough!

SpanContext spanContext = span.getSpanContext();
spanData.put(SPAN_ID, spanContext.getSpanId());
Expand Down
7 changes: 2 additions & 5 deletions ...lemetry/runtime/src/main/java/io/quarkus/opentelemetry/runtime/QuarkusContextStorage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
import static io.quarkus.vertx.core.runtime.context.VertxContextSafetyToggle.setContextSafe;
import static io.smallrye.common.vertx.VertxContext.isDuplicatedContext;

import java.util.Map;

import org.jboss.logging.Logger;

import io.opentelemetry.context.Context;
Expand Down Expand Up @@ -66,8 +64,7 @@ public Scope attach(io.vertx.core.Context vertxContext, Context toAttach) {
return Scope.noop();
}
vertxContext.putLocal(OTEL_CONTEXT, toAttach);
final Map<String, String> spanDataToAttach = OpenTelemetryUtil.getSpanData(toAttach);
OpenTelemetryUtil.setMDCData(spanDataToAttach, vertxContext);
OpenTelemetryUtil.setMDCData(toAttach, vertxContext);

return new Scope() {

Expand All @@ -77,7 +74,7 @@ public void close() {
if (before != toAttach) {
log.info("Context in storage not the expected context, Scope.close was not called correctly. Details:" +
" OTel context before: " + OpenTelemetryUtil.getSpanData(before) +
". OTel context toAttach: " + spanDataToAttach);
". OTel context toAttach: " + OpenTelemetryUtil.getSpanData(toAttach));
}

if (beforeAttach == null) {
Expand Down
3 changes: 1 addition & 2 deletions ...ry/runtime/src/main/java/io/quarkus/opentelemetry/runtime/tracing/DropTargetsSampler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,8 @@ public SamplingResult shouldSample(Context parentContext, String traceId, String

if (spanKind.equals(SpanKind.SERVER)) {
// HTTP_TARGET was split into url.path and url.query
String path = attributes.get(URL_PATH);
String query = attributes.get(URL_QUERY);
String target = path + (query == null ? "" : "?" + query);
String target = attributes.get(URL_PATH) + (query == null ? "" : "?" + query);

if (shouldDrop(target)) {
return SamplingResult.drop();
Expand Down
60 changes: 40 additions & 20 deletions ...rc/main/java/io/quarkus/opentelemetry/runtime/tracing/intrumentation/vertx/VertxUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ public final class VertxUtil {
private static final Pattern FORWARDED_FOR_PATTERN = Pattern.compile("for=\"?([^;,\"]+)\"?");
private static final String FORWARDED = "Forwarded";
private static final String COMMA_SPLITTER = ",";
private static final String COLON_SPLITTER = ":";
private static final char COLON_SPLITTER = ':';
private static final int SPLIT_LIMIT = -1;

private VertxUtil() {
Expand All @@ -35,23 +35,6 @@ private static String getXForwardedHeaderValue(HttpServerRequest httpServerReque
return xForwardedForHeader.split(COMMA_SPLITTER, SPLIT_LIMIT)[0];
}

private static String getHostHeader(HttpServerRequest httpRequest) {
String header = httpRequest.getHeader("host");
if (header == null) {
return null;
}
return header.split(COLON_SPLITTER, SPLIT_LIMIT)[0];
}

private static String getHostPortHeader(HttpServerRequest httpRequest) {
String header = httpRequest.getHeader("host");
if (header == null) {
return null;
}
String[] headerValues = header.split(COLON_SPLITTER, SPLIT_LIMIT);
return headerValues.length > 1 ? headerValues[1] : null;
}

public static String extractClientIP(HttpServerRequest httpServerRequest) {
// Tries to fetch Forwarded first since X-Forwarded can be lost by a proxy
// If Forwarded is not there tries to fetch the X-Forwarded-For header
Expand All @@ -69,15 +52,29 @@ public static String extractClientIP(HttpServerRequest httpServerRequest) {
}

public static String extractRemoteHostname(HttpServerRequest httpRequest) {
String hostname = getHostHeader(httpRequest);
String header = httpRequest.getHeader("host");
if (header == null) {
return null;
}

// localhost:8808, hostname localhost
String hostname = beforeDelimiter(header, COLON_SPLITTER);

if (hostname != null) {
return hostname;
}
return httpRequest.remoteAddress() != null ? httpRequest.remoteAddress().hostName() : null;
}

public static Long extractRemoteHostPort(HttpServerRequest httpRequest) {
String portString = getHostPortHeader(httpRequest);
String header = httpRequest.getHeader("host");
if (header == null) {
return null;
}

// localhost:8808, port 8080
String portString = afterDelimiter(header, COLON_SPLITTER);

if (portString != null) {
try {
return Long.parseLong(portString);
Expand All @@ -90,4 +87,27 @@ public static Long extractRemoteHostPort(HttpServerRequest httpRequest) {
}
return null;
}

private static String beforeDelimiter(String str, char delimiter) {
if (str == null || str.isEmpty()) {
return "";
}
int index = str.indexOf(delimiter);
return (index >= 0) ? str.substring(0, index) : str;
}

private static String afterDelimiter(String str, char delimiter) {
if (str == null || str.isEmpty()) {
return "";
}

int index = str.indexOf(delimiter);
if (index < 0) {
return null;
} else if (index >= 0 && index + 1 < str.length()) {
return str.substring(index + 1);
} else {
return "";
}
}
}
Loading
Loading