Skip to content
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.

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

Add builders for setting optional attributes on HTTP extractors #5347

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,9 @@ public class InstrumenterBenchmark {
"benchmark",
HttpSpanNameExtractor.create(ConstantHttpAttributesGetter.INSTANCE))
.addAttributesExtractor(
HttpClientAttributesExtractor.create(
ConstantHttpAttributesGetter.INSTANCE, CapturedHttpHeaders.empty()))
HttpClientAttributesExtractor.builder(ConstantHttpAttributesGetter.INSTANCE)
.captureHttpHeaders(CapturedHttpHeaders.empty())
.build())
.addAttributesExtractor(
NetServerAttributesExtractor.create(new ConstantNetAttributesGetter()))
.newInstrumenter();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import io.opentelemetry.api.common.AttributesBuilder;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.config.Config;
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
import javax.annotation.Nullable;

Expand All @@ -27,23 +26,33 @@ public final class HttpClientAttributesExtractor<REQUEST, RESPONSE>
/** Creates the HTTP client attributes extractor with default configuration. */
public static <REQUEST, RESPONSE> HttpClientAttributesExtractor<REQUEST, RESPONSE> create(
HttpClientAttributesGetter<REQUEST, RESPONSE> getter) {
return create(getter, CapturedHttpHeaders.client(Config.get()));
return builder(getter).build();
}

// TODO: there should be a builder for all optional attributes
/**
* Creates the HTTP client attributes extractor.
*
* @param capturedHttpHeaders A configuration object specifying which HTTP request and response
* headers should be captured as span attributes.
* @deprecated Use {@link #builder(HttpClientAttributesGetter)} instead.
*/
@Deprecated
public static <REQUEST, RESPONSE> HttpClientAttributesExtractor<REQUEST, RESPONSE> create(
HttpClientAttributesGetter<REQUEST, RESPONSE> getter,
CapturedHttpHeaders capturedHttpHeaders) {
return new HttpClientAttributesExtractor<>(getter, capturedHttpHeaders);
return builder(getter).captureHttpHeaders(capturedHttpHeaders).build();
}

private HttpClientAttributesExtractor(
/**
* Returns a new {@link HttpClientAttributesExtractorBuilder} that can be used to configure the
* HTTP client attributes extractor.
*/
public static <REQUEST, RESPONSE> HttpClientAttributesExtractorBuilder<REQUEST, RESPONSE> builder(
HttpClientAttributesGetter<REQUEST, RESPONSE> getter) {
return new HttpClientAttributesExtractorBuilder<>(getter);
}

HttpClientAttributesExtractor(
HttpClientAttributesGetter<REQUEST, RESPONSE> getter,
CapturedHttpHeaders capturedHttpHeaders) {
super(getter, capturedHttpHeaders);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.instrumenter.http;

import io.opentelemetry.instrumentation.api.config.Config;

/** A builder of {@link HttpClientAttributesExtractor}. */
public final class HttpClientAttributesExtractorBuilder<REQUEST, RESPONSE> {

final HttpClientAttributesGetter<REQUEST, RESPONSE> getter;
CapturedHttpHeaders capturedHttpHeaders = CapturedHttpHeaders.client(Config.get());

HttpClientAttributesExtractorBuilder(HttpClientAttributesGetter<REQUEST, RESPONSE> getter) {
this.getter = getter;
}

/**
* Configures the HTTP headers that will be captured as span attributes.
*
* @param capturedHttpHeaders A configuration object specifying which HTTP request and response
* headers should be captured as span attributes.
*/
public HttpClientAttributesExtractorBuilder<REQUEST, RESPONSE> captureHttpHeaders(
CapturedHttpHeaders capturedHttpHeaders) {
this.capturedHttpHeaders = capturedHttpHeaders;
return this;
}

/**
* Returns a new {@link HttpClientAttributesExtractor} with the settings of this {@link
* HttpClientAttributesExtractorBuilder}.
*/
public HttpClientAttributesExtractor<REQUEST, RESPONSE> build() {
return new HttpClientAttributesExtractor<>(getter, capturedHttpHeaders);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@

import io.opentelemetry.api.common.AttributesBuilder;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.config.Config;
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
import java.util.function.Function;
import javax.annotation.Nullable;
Expand All @@ -33,25 +32,40 @@ public final class HttpServerAttributesExtractor<REQUEST, RESPONSE>
/** Creates the HTTP server attributes extractor with default configuration. */
public static <REQUEST, RESPONSE> HttpServerAttributesExtractor<REQUEST, RESPONSE> create(
HttpServerAttributesGetter<REQUEST, RESPONSE> getter) {
return create(getter, CapturedHttpHeaders.server(Config.get()));
return builder(getter).build();
}

// TODO: there should be a builder for all optional attributes
/**
* Creates the HTTP server attributes extractor.
*
* @param capturedHttpHeaders A configuration object specifying which HTTP request and response
* headers should be captured as span attributes.
* @deprecated Use {@link #builder(HttpServerAttributesGetter)} instead.
*/
@Deprecated
public static <REQUEST, RESPONSE> HttpServerAttributesExtractor<REQUEST, RESPONSE> create(
HttpServerAttributesGetter<REQUEST, RESPONSE> getter,
CapturedHttpHeaders capturedHttpHeaders) {
return new HttpServerAttributesExtractor<>(
getter, capturedHttpHeaders, HttpRouteHolder::getRoute);
return builder(getter).captureHttpHeaders(capturedHttpHeaders).build();
}

/**
* Returns a new {@link HttpServerAttributesExtractorBuilder} that can be used to configure the
* HTTP client attributes extractor.
*/
public static <REQUEST, RESPONSE> HttpServerAttributesExtractorBuilder<REQUEST, RESPONSE> builder(
HttpServerAttributesGetter<REQUEST, RESPONSE> getter) {
return new HttpServerAttributesExtractorBuilder<>(getter);
}

private final Function<Context, String> httpRouteHolderGetter;

HttpServerAttributesExtractor(
HttpServerAttributesGetter<REQUEST, RESPONSE> getter,
CapturedHttpHeaders capturedHttpHeaders) {
this(getter, capturedHttpHeaders, HttpRouteHolder::getRoute);
}

// visible for tests
HttpServerAttributesExtractor(
HttpServerAttributesGetter<REQUEST, RESPONSE> getter,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.instrumenter.http;

import io.opentelemetry.instrumentation.api.config.Config;

/** A builder of {@link HttpServerAttributesExtractor}. */
public final class HttpServerAttributesExtractorBuilder<REQUEST, RESPONSE> {

final HttpServerAttributesGetter<REQUEST, RESPONSE> getter;
CapturedHttpHeaders capturedHttpHeaders = CapturedHttpHeaders.server(Config.get());

HttpServerAttributesExtractorBuilder(HttpServerAttributesGetter<REQUEST, RESPONSE> getter) {
this.getter = getter;
}

/**
* Configures the HTTP headers that will be captured as span attributes.
*
* @param capturedHttpHeaders A configuration object specifying which HTTP request and response
* headers should be captured as span attributes.
*/
public HttpServerAttributesExtractorBuilder<REQUEST, RESPONSE> captureHttpHeaders(
CapturedHttpHeaders capturedHttpHeaders) {
this.capturedHttpHeaders = capturedHttpHeaders;
return this;
}

/**
* Returns a new {@link HttpServerAttributesExtractor} with the settings of this {@link
* HttpServerAttributesExtractorBuilder}.
*/
public HttpServerAttributesExtractor<REQUEST, RESPONSE> build() {
return new HttpServerAttributesExtractor<>(getter, capturedHttpHeaders);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,12 @@ void normal() {
response.put("header.custom-response-header", "654,321");

HttpClientAttributesExtractor<Map<String, String>, Map<String, String>> extractor =
HttpClientAttributesExtractor.create(
new TestHttpClientAttributesGetter(),
CapturedHttpHeaders.create(
singletonList("Custom-Request-Header"), singletonList("Custom-Response-Header")));
HttpClientAttributesExtractor.builder(new TestHttpClientAttributesGetter())
.captureHttpHeaders(
CapturedHttpHeaders.create(
singletonList("Custom-Request-Header"),
singletonList("Custom-Response-Header")))
.build();
Comment on lines +107 to +112
Copy link
Member

Choose a reason for hiding this comment

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

what do you think of hiding CapturedHttpHeaders and exposing captureHttpRequestHeaders and captureHttpResponseHeaders on the builders?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'd probably prefer leaving this as it is, it's a bit simpler this way.

If I added captureHttpRequestHeaders(List<String>) I'd have to take the following things into account:

  • is it additive? does . captureHttpRequestHeaders(["abc"]).captureHttpRequestHeaders(["def"]) capture ["abc", "def"] or does the second call overwrite the first one?
  • do the default (configured) values get overwritten, or appended to?

With captureHttpHeaders(CapturedHttpHeaders) it's clear (I hope so? 😅 ) that the previous setting is overwritten. (Maybe I should rename it to setCapturedHttpHeaders after all...)

Copy link
Member

Choose a reason for hiding this comment

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

cc @anuraaga for any API thoughts

Copy link
Contributor

Choose a reason for hiding this comment

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

How about

setCapturedRequestHeaders
setCapturedResponseHeaders

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll do that in a separate PR - this requires changing all our HTTP library instrumentations, so the amount of changes will be pretty significant.


AttributesBuilder attributes = Attributes.builder();
extractor.onStart(attributes, Context.root(), request);
Expand Down Expand Up @@ -148,8 +150,9 @@ void invalidStatusCode() {
response.put("statusCode", "0");

HttpClientAttributesExtractor<Map<String, String>, Map<String, String>> extractor =
HttpClientAttributesExtractor.create(
new TestHttpClientAttributesGetter(), CapturedHttpHeaders.empty());
HttpClientAttributesExtractor.builder(new TestHttpClientAttributesGetter())
.captureHttpHeaders(CapturedHttpHeaders.empty())
.build();

AttributesBuilder attributes = Attributes.builder();
extractor.onStart(attributes, Context.root(), request);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,9 @@ void extractClientIpFromX_Forwarded_For() {
request.put("header.x-forwarded-for", "1.1.1.1");

HttpServerAttributesExtractor<Map<String, String>, Map<String, String>> extractor =
HttpServerAttributesExtractor.create(
new TestHttpServerAttributesExtractor(), CapturedHttpHeaders.empty());
HttpServerAttributesExtractor.builder(new TestHttpServerAttributesExtractor())
.captureHttpHeaders(CapturedHttpHeaders.empty())
.build();

AttributesBuilder attributes = Attributes.builder();
extractor.onStart(attributes, Context.root(), request);
Expand All @@ -203,8 +204,9 @@ void extractClientIpFromX_Forwarded_Proto() {
request.put("header.x-forwarded-proto", "https");

HttpServerAttributesExtractor<Map<String, String>, Map<String, String>> extractor =
HttpServerAttributesExtractor.create(
new TestHttpServerAttributesExtractor(), CapturedHttpHeaders.empty());
HttpServerAttributesExtractor.builder(new TestHttpServerAttributesExtractor())
.captureHttpHeaders(CapturedHttpHeaders.empty())
.build();

AttributesBuilder attributes = Attributes.builder();
extractor.onStart(attributes, Context.root(), request);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,9 @@ public ApacheHttpClientTracing build() {
HttpSpanNameExtractor.create(httpAttributesGetter))
.setSpanStatusExtractor(HttpSpanStatusExtractor.create(httpAttributesGetter))
.addAttributesExtractor(
HttpClientAttributesExtractor.create(httpAttributesGetter, capturedHttpHeaders))
HttpClientAttributesExtractor.builder(httpAttributesGetter)
.captureHttpHeaders(capturedHttpHeaders)
.build())
.addAttributesExtractor(NetClientAttributesExtractor.create(netAttributesGetter))
.addAttributesExtractors(additionalExtractors)
// We manually inject because we need to inject internal requests for redirects.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,9 @@ public ArmeriaTracing build() {
HttpSpanStatusExtractor.create(clientAttributesGetter)))
.addAttributesExtractor(netClientAttributesExtractor)
.addAttributesExtractor(
HttpClientAttributesExtractor.create(clientAttributesGetter, capturedHttpClientHeaders))
HttpClientAttributesExtractor.builder(clientAttributesGetter)
.captureHttpHeaders(capturedHttpClientHeaders)
.build())
.addRequestMetrics(HttpClientMetrics.get());
serverInstrumenterBuilder
.setSpanStatusExtractor(
Expand All @@ -144,7 +146,9 @@ public ArmeriaTracing build() {
.addAttributesExtractor(
NetServerAttributesExtractor.create(new ArmeriaNetServerAttributesGetter()))
.addAttributesExtractor(
HttpServerAttributesExtractor.create(serverAttributesGetter, capturedHttpServerHeaders))
HttpServerAttributesExtractor.builder(serverAttributesGetter)
.captureHttpHeaders(capturedHttpServerHeaders)
.build())
.addRequestMetrics(HttpServerMetrics.get())
.addContextCustomizer(HttpRouteHolder.get());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ public Instrumenter<Request, Response> build() {
HttpSpanNameExtractor.create(httpAttributesGetter))
.setSpanStatusExtractor(HttpSpanStatusExtractor.create(httpAttributesGetter))
.addAttributesExtractor(
HttpClientAttributesExtractor.create(httpAttributesGetter, capturedHttpHeaders))
HttpClientAttributesExtractor.builder(httpAttributesGetter)
.captureHttpHeaders(capturedHttpHeaders)
.build())
.addAttributesExtractor(NetClientAttributesExtractor.create(netAttributesGetter))
.addAttributesExtractors(additionalExtractors)
.addRequestMetrics(HttpClientMetrics.get())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ class KtorServerTracing private constructor(
with(instrumenterBuilder) {
setSpanStatusExtractor(configuration.statusExtractor(HttpSpanStatusExtractor.create(httpAttributesGetter)))
addAttributesExtractor(NetServerAttributesExtractor.create(KtorNetServerAttributesGetter()))
addAttributesExtractor(HttpServerAttributesExtractor.create(httpAttributesGetter, configuration.capturedHttpHeaders))
addAttributesExtractor(HttpServerAttributesExtractor.builder(httpAttributesGetter).captureHttpHeaders(configuration.capturedHttpHeaders).build())
addRequestMetrics(HttpServerMetrics.get())
addContextCustomizer(HttpRouteHolder.get())
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,9 @@ public OkHttpTracing build() {
HttpSpanNameExtractor.create(httpAttributesGetter))
.setSpanStatusExtractor(HttpSpanStatusExtractor.create(httpAttributesGetter))
.addAttributesExtractor(
HttpClientAttributesExtractor.create(httpAttributesGetter, capturedHttpHeaders))
HttpClientAttributesExtractor.builder(httpAttributesGetter)
.captureHttpHeaders(capturedHttpHeaders)
.build())
.addAttributesExtractor(NetClientAttributesExtractor.create(attributesGetter))
.addAttributesExtractors(additionalExtractors)
.addRequestMetrics(HttpClientMetrics.get())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,9 @@ public RatpackTracing build() {
.setSpanStatusExtractor(HttpSpanStatusExtractor.create(httpAttributes))
.addAttributesExtractor(NetServerAttributesExtractor.create(netAttributes))
.addAttributesExtractor(
HttpServerAttributesExtractor.create(httpAttributes, capturedHttpServerHeaders))
HttpServerAttributesExtractor.builder(httpAttributes)
.captureHttpHeaders(capturedHttpServerHeaders)
.build())
.addAttributesExtractors(additionalExtractors)
.addRequestMetrics(HttpServerMetrics.get())
.newServerInstrumenter(RatpackGetter.INSTANCE);
Expand All @@ -128,7 +130,9 @@ private Instrumenter<RequestSpec, HttpResponse> httpClientInstrumenter() {
.setSpanStatusExtractor(HttpSpanStatusExtractor.create(httpAttributes))
.addAttributesExtractor(NetClientAttributesExtractor.create(netAttributes))
.addAttributesExtractor(
HttpClientAttributesExtractor.create(httpAttributes, capturedHttpClientHeaders))
HttpClientAttributesExtractor.builder(httpAttributes)
.captureHttpHeaders(capturedHttpClientHeaders)
.build())
.addAttributesExtractors(additionalHttpClientExtractors)
.addRequestMetrics(HttpServerMetrics.get())
.newClientInstrumenter(RequestHeaderSetter.INSTANCE);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,9 @@ public RestletTracing build() {
HttpSpanNameExtractor.create(httpAttributesGetter))
.setSpanStatusExtractor(HttpSpanStatusExtractor.create(httpAttributesGetter))
.addAttributesExtractor(
HttpServerAttributesExtractor.create(httpAttributesGetter, capturedHttpHeaders))
HttpServerAttributesExtractor.builder(httpAttributesGetter)
.captureHttpHeaders(capturedHttpHeaders)
.build())
.addAttributesExtractor(NetServerAttributesExtractor.create(netAttributesGetter))
.addAttributesExtractors(additionalExtractors)
.addRequestMetrics(HttpServerMetrics.get())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ public static Instrumenter<Request, Response> newServerInstrumenter(
openTelemetry, INSTRUMENTATION_NAME, HttpSpanNameExtractor.create(httpAttributesGetter))
.setSpanStatusExtractor(HttpSpanStatusExtractor.create(httpAttributesGetter))
.addAttributesExtractor(
HttpServerAttributesExtractor.create(httpAttributesGetter, capturedHttpHeaders))
HttpServerAttributesExtractor.builder(httpAttributesGetter)
.captureHttpHeaders(capturedHttpHeaders)
.build())
.addAttributesExtractor(NetServerAttributesExtractor.create(netAttributesGetter))
.addAttributesExtractors(additionalExtractors)
.addRequestMetrics(HttpServerMetrics.get())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,9 @@ public SpringWebTracing build() {
HttpSpanNameExtractor.create(httpAttributeGetter))
.setSpanStatusExtractor(HttpSpanStatusExtractor.create(httpAttributeGetter))
.addAttributesExtractor(
HttpClientAttributesExtractor.create(httpAttributeGetter, capturedHttpHeaders))
HttpClientAttributesExtractor.builder(httpAttributeGetter)
.captureHttpHeaders(capturedHttpHeaders)
.build())
.addAttributesExtractor(NetClientAttributesExtractor.create(netAttributesGetter))
.addAttributesExtractors(additionalExtractors)
.addRequestMetrics(HttpClientMetrics.get())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,9 @@ public SpringWebfluxTracing build() {
HttpSpanNameExtractor.create(httpAttributesGetter))
.setSpanStatusExtractor(HttpSpanStatusExtractor.create(httpAttributesGetter))
.addAttributesExtractor(
HttpClientAttributesExtractor.create(httpAttributesGetter, capturedHttpHeaders))
HttpClientAttributesExtractor.builder(httpAttributesGetter)
.captureHttpHeaders(capturedHttpHeaders)
.build())
.addAttributesExtractor(attributesExtractor)
.addAttributesExtractor(PeerServiceAttributesExtractor.create(attributesGetter))
.addAttributesExtractors(additionalExtractors)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,9 @@ public SpringWebMvcTracing build() {
HttpSpanNameExtractor.create(httpAttributesGetter))
.setSpanStatusExtractor(HttpSpanStatusExtractor.create(httpAttributesGetter))
.addAttributesExtractor(
HttpServerAttributesExtractor.create(httpAttributesGetter, capturedHttpHeaders))
HttpServerAttributesExtractor.builder(httpAttributesGetter)
.captureHttpHeaders(capturedHttpHeaders)
.build())
.addAttributesExtractor(new StatusCodeExtractor())
.addAttributesExtractor(
NetServerAttributesExtractor.create(new SpringWebMvcNetAttributesGetter()))
Expand Down