Skip to content

Conversation

brunobat
Copy link
Contributor

@brunobat brunobat commented May 9, 2025

During the otel performance work some inefficiencies were detected on the hot path, the one affecting the creation of all spans:

  1. We were placing MDC trace data in a HashMap that was resizing. Code was re-written in order to not use the HashMap at all.
  2. When calculating host and port we where using String.split() which uses a regex and was costing many cycles. Re-wrote that part and added tests to make sure behaviour is compatible.

@brunobat brunobat requested a review from geoand May 9, 2025 07:50
Copy link

quarkus-bot bot commented May 9, 2025

/cc @radcortez (opentelemetry)

Copy link

quarkus-bot bot commented May 9, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit c65d7ac.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

LGTM

@brunobat brunobat merged commit 73e1d2e into quarkusio:main May 9, 2025
34 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.23 - main milestone May 9, 2025
@brunobat
Copy link
Contributor Author

FYI, After profiling, the test run used 4% less samples.

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

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

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants