Skip to content

Commit

Permalink
Don't normalize the '-' character in HTTP header names (#9735)
Browse files Browse the repository at this point in the history
  • Loading branch information
Mateusz Rzeszutek committed Oct 24, 2023
1 parent 5a2f529 commit 8bc5297
Show file tree
Hide file tree
Showing 10 changed files with 148 additions and 42 deletions.
46 changes: 34 additions & 12 deletions .../java/io/opentelemetry/instrumentation/api/instrumenter/http/CapturedHttpHeadersUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,31 +17,53 @@
final class CapturedHttpHeadersUtil {

// these are naturally bounded because they only store keys listed in
// otel.instrumentation.http.capture-headers.server.request and
// otel.instrumentation.http.capture-headers.server.response
private static final ConcurrentMap<String, AttributeKey<List<String>>> requestKeysCache =
new ConcurrentHashMap<>();
private static final ConcurrentMap<String, AttributeKey<List<String>>> responseKeysCache =
new ConcurrentHashMap<>();
// otel.instrumentation.http.server.capture-request-headers and
// otel.instrumentation.http.server.capture-response-headers
private static final ConcurrentMap<String, AttributeKey<List<String>>>
oldSemconvRequestKeysCache = new ConcurrentHashMap<>();
private static final ConcurrentMap<String, AttributeKey<List<String>>>
stableSemconvRequestKeysCache = new ConcurrentHashMap<>();
private static final ConcurrentMap<String, AttributeKey<List<String>>>
oldSemconvResponseKeysCache = new ConcurrentHashMap<>();
private static final ConcurrentMap<String, AttributeKey<List<String>>>
stableSemconvResponseKeysCache = new ConcurrentHashMap<>();

static List<String> lowercase(List<String> names) {
return unmodifiableList(
names.stream().map(s -> s.toLowerCase(Locale.ROOT)).collect(Collectors.toList()));
}

static AttributeKey<List<String>> requestAttributeKey(String headerName) {
return requestKeysCache.computeIfAbsent(headerName, n -> createKey("request", n));
static AttributeKey<List<String>> oldSemconvRequestAttributeKey(String headerName) {
return oldSemconvRequestKeysCache.computeIfAbsent(
headerName, n -> createOldSemconvKey("request", n));
}

static AttributeKey<List<String>> responseAttributeKey(String headerName) {
return responseKeysCache.computeIfAbsent(headerName, n -> createKey("response", n));
static AttributeKey<List<String>> stableSemconvRequestAttributeKey(String headerName) {
return stableSemconvRequestKeysCache.computeIfAbsent(
headerName, n -> createStableSemconvKey("request", n));
}

private static AttributeKey<List<String>> createKey(String type, String headerName) {
// headerName is always lowercase, see CapturedHttpHeaders
static AttributeKey<List<String>> oldSemconvResponseAttributeKey(String headerName) {
return oldSemconvResponseKeysCache.computeIfAbsent(
headerName, n -> createOldSemconvKey("response", n));
}

static AttributeKey<List<String>> stableSemconvResponseAttributeKey(String headerName) {
return stableSemconvResponseKeysCache.computeIfAbsent(
headerName, n -> createStableSemconvKey("response", n));
}

private static AttributeKey<List<String>> createOldSemconvKey(String type, String headerName) {
// headerName is always lowercase, see CapturedHttpHeadersUtil#lowercase
String key = "http." + type + ".header." + headerName.replace('-', '_');
return AttributeKey.stringArrayKey(key);
}

private static AttributeKey<List<String>> createStableSemconvKey(String type, String headerName) {
// headerName is always lowercase, see CapturedHttpHeadersUtil#lowercase
String key = "http." + type + ".header." + headerName;
return AttributeKey.stringArrayKey(key);
}

private CapturedHttpHeadersUtil() {}
}
20 changes: 16 additions & 4 deletions ...io/opentelemetry/instrumentation/api/instrumenter/http/HttpCommonAttributesExtractor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@
package io.opentelemetry.instrumentation.api.instrumenter.http;

import static io.opentelemetry.instrumentation.api.instrumenter.http.CapturedHttpHeadersUtil.lowercase;
import static io.opentelemetry.instrumentation.api.instrumenter.http.CapturedHttpHeadersUtil.requestAttributeKey;
import static io.opentelemetry.instrumentation.api.instrumenter.http.CapturedHttpHeadersUtil.responseAttributeKey;
import static io.opentelemetry.instrumentation.api.instrumenter.http.CapturedHttpHeadersUtil.oldSemconvRequestAttributeKey;
import static io.opentelemetry.instrumentation.api.instrumenter.http.CapturedHttpHeadersUtil.oldSemconvResponseAttributeKey;
import static io.opentelemetry.instrumentation.api.instrumenter.http.CapturedHttpHeadersUtil.stableSemconvRequestAttributeKey;
import static io.opentelemetry.instrumentation.api.instrumenter.http.CapturedHttpHeadersUtil.stableSemconvResponseAttributeKey;
import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.internalSet;
import static io.opentelemetry.instrumentation.api.internal.HttpConstants._OTHER;

Expand Down Expand Up @@ -70,7 +72,12 @@ public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST
for (String name : capturedRequestHeaders) {
List<String> values = getter.getHttpRequestHeader(request, name);
if (!values.isEmpty()) {
internalSet(attributes, requestAttributeKey(name), values);
if (SemconvStability.emitOldHttpSemconv()) {
internalSet(attributes, oldSemconvRequestAttributeKey(name), values);
}
if (SemconvStability.emitStableHttpSemconv()) {
internalSet(attributes, stableSemconvRequestAttributeKey(name), values);
}
}
}
}
Expand Down Expand Up @@ -115,7 +122,12 @@ public void onEnd(
for (String name : capturedResponseHeaders) {
List<String> values = getter.getHttpResponseHeader(request, response, name);
if (!values.isEmpty()) {
internalSet(attributes, responseAttributeKey(name), values);
if (SemconvStability.emitOldHttpSemconv()) {
internalSet(attributes, oldSemconvResponseAttributeKey(name), values);
}
if (SemconvStability.emitStableHttpSemconv()) {
internalSet(attributes, stableSemconvResponseAttributeKey(name), values);
}
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions ...y/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorBothSemconvTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,9 @@ void normal() {
entry(
AttributeKey.stringArrayKey("http.request.header.custom_request_header"),
asList("123", "456")),
entry(
AttributeKey.stringArrayKey("http.request.header.custom-request-header"),
asList("123", "456")),
entry(SemanticAttributes.NET_PEER_NAME, "github.com"),
entry(SemanticAttributes.NET_PEER_PORT, 123L),
entry(SemanticAttributes.SERVER_ADDRESS, "github.com"),
Expand All @@ -162,6 +165,9 @@ void normal() {
entry(
AttributeKey.stringArrayKey("http.response.header.custom_response_header"),
asList("654", "321")),
entry(
AttributeKey.stringArrayKey("http.response.header.custom-response-header"),
asList("654", "321")),
entry(SemanticAttributes.NET_PROTOCOL_NAME, "http"),
entry(SemanticAttributes.NET_PROTOCOL_VERSION, "1.1"),
entry(SemanticAttributes.NETWORK_PROTOCOL_NAME, "http"),
Expand Down
6 changes: 6 additions & 0 deletions ...y/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorBothSemconvTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,9 @@ void normal() {
entry(SemanticAttributes.CLIENT_ADDRESS, "1.1.1.1"),
entry(
AttributeKey.stringArrayKey("http.request.header.custom_request_header"),
asList("123", "456")),
entry(
AttributeKey.stringArrayKey("http.request.header.custom-request-header"),
asList("123", "456")));

AttributesBuilder endAttributes = Attributes.builder();
Expand All @@ -189,6 +192,9 @@ void normal() {
entry(SemanticAttributes.HTTP_RESPONSE_BODY_SIZE, 20L),
entry(
AttributeKey.stringArrayKey("http.response.header.custom_response_header"),
asList("654", "321")),
entry(
AttributeKey.stringArrayKey("http.response.header.custom-response-header"),
asList("654", "321")));
}
}
4 changes: 2 additions & 2 deletions ...instrumentation/api/instrumenter/http/HttpClientAttributesExtractorStableSemconvTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ void normal() {
entry(SemanticAttributes.URL_FULL, "http://github.com"),
entry(SemanticAttributes.USER_AGENT_ORIGINAL, "okhttp 3.x"),
entry(
AttributeKey.stringArrayKey("http.request.header.custom_request_header"),
AttributeKey.stringArrayKey("http.request.header.custom-request-header"),
asList("123", "456")),
entry(SemanticAttributes.SERVER_ADDRESS, "github.com"),
entry(SemanticAttributes.SERVER_PORT, 123L),
Expand All @@ -187,7 +187,7 @@ void normal() {
entry(SemanticAttributes.HTTP_RESPONSE_STATUS_CODE, 202L),
entry(SemanticAttributes.HTTP_RESPONSE_BODY_SIZE, 20L),
entry(
AttributeKey.stringArrayKey("http.response.header.custom_response_header"),
AttributeKey.stringArrayKey("http.response.header.custom-response-header"),
asList("654", "321")),
entry(SemanticAttributes.NETWORK_PROTOCOL_NAME, "http"),
entry(SemanticAttributes.NETWORK_PROTOCOL_VERSION, "1.1"),
Expand Down
4 changes: 2 additions & 2 deletions ...instrumentation/api/instrumenter/http/HttpServerAttributesExtractorStableSemconvTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ void normal() {
entry(SemanticAttributes.HTTP_ROUTE, "/repositories/{id}"),
entry(SemanticAttributes.CLIENT_ADDRESS, "1.1.1.1"),
entry(
AttributeKey.stringArrayKey("http.request.header.custom_request_header"),
AttributeKey.stringArrayKey("http.request.header.custom-request-header"),
asList("123", "456")));

AttributesBuilder endAttributes = Attributes.builder();
Expand All @@ -235,7 +235,7 @@ void normal() {
entry(SemanticAttributes.HTTP_RESPONSE_STATUS_CODE, 202L),
entry(SemanticAttributes.HTTP_RESPONSE_BODY_SIZE, 20L),
entry(
AttributeKey.stringArrayKey("http.response.header.custom_response_header"),
AttributeKey.stringArrayKey("http.response.header.custom-response-header"),
asList("654", "321")));
}

Expand Down
11 changes: 9 additions & 2 deletions ...rumentation/jaxrs/jaxrs-common/testing/src/main/groovy/AbstractJaxRsHttpServerTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

import io.opentelemetry.instrumentation.api.internal.SemconvStability
import io.opentelemetry.instrumentation.test.AgentTestTrait
import io.opentelemetry.instrumentation.test.asserts.TraceAssert
import io.opentelemetry.instrumentation.test.base.HttpServerTest
Expand Down Expand Up @@ -296,8 +297,14 @@ abstract class AbstractJaxRsHttpServerTest<S> extends HttpServerTest<S> implemen
"$SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH" { it == null || it instanceof Long }
"$SemanticAttributes.HTTP_ROUTE" path
if (fullUrl.getPath().endsWith(ServerEndpoint.CAPTURE_HEADERS.getPath())) {
"http.request.header.x_test_request" { it == ["test"] }
"http.response.header.x_test_response" { it == ["test"] }
if (SemconvStability.emitOldHttpSemconv()) {
"http.request.header.x_test_request" { it == ["test"] }
"http.response.header.x_test_response" { it == ["test"] }
}
if (SemconvStability.emitStableHttpSemconv()) {
"http.request.header.x-test-request" { it == ["test"] }
"http.response.header.x-test-response" { it == ["test"] }
}
}
}
}
Expand Down
34 changes: 28 additions & 6 deletions ...io/opentelemetry/javaagent/instrumentation/servlet/ServletRequestParametersExtractor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import io.opentelemetry.api.common.AttributesBuilder;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
import io.opentelemetry.instrumentation.api.internal.SemconvStability;
import io.opentelemetry.javaagent.bootstrap.internal.InstrumentationConfig;
import java.util.List;
import java.util.Locale;
Expand All @@ -27,8 +28,10 @@ public class ServletRequestParametersExtractor<REQUEST, RESPONSE>
.getList(
"otel.instrumentation.servlet.experimental.capture-request-parameters", emptyList());

private static final ConcurrentMap<String, AttributeKey<List<String>>> parameterKeysCache =
new ConcurrentHashMap<>();
private static final ConcurrentMap<String, AttributeKey<List<String>>>
oldSemconvParameterKeysCache = new ConcurrentHashMap<>();
private static final ConcurrentMap<String, AttributeKey<List<String>>>
stableSemconvParameterKeysCache = new ConcurrentHashMap<>();

private final ServletAccessor<REQUEST, RESPONSE> accessor;

Expand All @@ -45,7 +48,12 @@ public void setAttributes(
for (String name : CAPTURE_REQUEST_PARAMETERS) {
List<String> values = accessor.getRequestParameterValues(request, name);
if (!values.isEmpty()) {
consumer.accept(parameterAttributeKey(name), values);
if (SemconvStability.emitOldHttpSemconv()) {
consumer.accept(oldSemconvParameterAttributeKey(name), values);
}
if (SemconvStability.emitStableHttpSemconv()) {
consumer.accept(stableSemconvParameterAttributeKey(name), values);
}
}
}
}
Expand All @@ -69,15 +77,29 @@ public void onEnd(
setAttributes(request, attributes::put);
}

private static AttributeKey<List<String>> parameterAttributeKey(String headerName) {
return parameterKeysCache.computeIfAbsent(headerName, n -> createKey(n));
private static AttributeKey<List<String>> oldSemconvParameterAttributeKey(String headerName) {
return oldSemconvParameterKeysCache.computeIfAbsent(
headerName, ServletRequestParametersExtractor::createOldSemconvKey);
}

private static AttributeKey<List<String>> createKey(String parameterName) {
private static AttributeKey<List<String>> stableSemconvParameterAttributeKey(String headerName) {
return stableSemconvParameterKeysCache.computeIfAbsent(
headerName, ServletRequestParametersExtractor::createStableSemconvKey);
}

private static AttributeKey<List<String>> createOldSemconvKey(String parameterName) {
// normalize parameter name similarly as is done with header names when header values are
// captured as span attributes
parameterName = parameterName.toLowerCase(Locale.ROOT);
String key = "servlet.request.parameter." + parameterName.replace('-', '_');
return AttributeKey.stringArrayKey(key);
}

private static AttributeKey<List<String>> createStableSemconvKey(String parameterName) {
// normalize parameter name similarly as is done with header names when header values are
// captured as span attributes
parameterName = parameterName.toLowerCase(Locale.ROOT);
String key = "servlet.request.parameter." + parameterName;
return AttributeKey.stringArrayKey(key);
}
}
30 changes: 23 additions & 7 deletions ...main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import io.opentelemetry.instrumentation.api.internal.SemconvStability;
import io.opentelemetry.instrumentation.test.utils.PortUtils;
import io.opentelemetry.instrumentation.testing.InstrumentationTestRunner;
import io.opentelemetry.sdk.testing.assertj.AttributeAssertion;
import io.opentelemetry.sdk.testing.assertj.SpanDataAssert;
import io.opentelemetry.sdk.testing.assertj.TraceAssert;
import io.opentelemetry.sdk.trace.data.SpanData;
Expand Down Expand Up @@ -538,13 +539,28 @@ void captureHttpHeaders() throws Exception {
trace.hasSpansSatisfyingExactly(
span -> {
assertClientSpan(span, uri, method, responseCode, null).hasNoParent();
span.hasAttributesSatisfying(
equalTo(
AttributeKey.stringArrayKey("http.request.header.x_test_request"),
singletonList("test")),
equalTo(
AttributeKey.stringArrayKey("http.response.header.x_test_response"),
singletonList("test")));
List<AttributeAssertion> attributeAssertions = new ArrayList<>();
if (SemconvStability.emitOldHttpSemconv()) {
attributeAssertions.add(
equalTo(
AttributeKey.stringArrayKey("http.request.header.x_test_request"),
singletonList("test")));
attributeAssertions.add(
equalTo(
AttributeKey.stringArrayKey("http.response.header.x_test_response"),
singletonList("test")));
}
if (SemconvStability.emitStableHttpSemconv()) {
attributeAssertions.add(
equalTo(
AttributeKey.stringArrayKey("http.request.header.x-test-request"),
singletonList("test")));
attributeAssertions.add(
equalTo(
AttributeKey.stringArrayKey("http.response.header.x-test-response"),
singletonList("test")));
}
span.hasAttributesSatisfying(attributeAssertions);
},
span -> assertServerSpan(span).hasParent(trace.getSpan(0)));
});
Expand Down
29 changes: 22 additions & 7 deletions ...main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpServerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -854,15 +854,30 @@ protected SpanDataAssert assertServerSpan(
}

if (endpoint == CAPTURE_HEADERS) {
assertThat(attrs)
.containsEntry("http.request.header.x_test_request", new String[] {"test"});
assertThat(attrs)
.containsEntry("http.response.header.x_test_response", new String[] {"test"});
if (SemconvStability.emitOldHttpSemconv()) {
assertThat(attrs)
.containsEntry("http.request.header.x_test_request", new String[] {"test"});
assertThat(attrs)
.containsEntry("http.response.header.x_test_response", new String[] {"test"});
}
if (SemconvStability.emitStableHttpSemconv()) {
assertThat(attrs)
.containsEntry("http.request.header.x-test-request", new String[] {"test"});
assertThat(attrs)
.containsEntry("http.response.header.x-test-response", new String[] {"test"});
}
}
if (endpoint == CAPTURE_PARAMETERS) {
assertThat(attrs)
.containsEntry(
"servlet.request.parameter.test_parameter", new String[] {"test value õäöü"});
if (SemconvStability.emitOldHttpSemconv()) {
assertThat(attrs)
.containsEntry(
"servlet.request.parameter.test_parameter", new String[] {"test value õäöü"});
}
if (SemconvStability.emitStableHttpSemconv()) {
assertThat(attrs)
.containsEntry(
"servlet.request.parameter.test-parameter", new String[] {"test value õäöü"});
}
}
});

Expand Down

0 comments on commit 8bc5297

Please sign in to comment.