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

Define url.scheme in terms of logical operation in HTTP server semconv #9698

Merged
merged 1 commit into from
Oct 17, 2023
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
4 changes: 0 additions & 4 deletions instrumentation-api-semconv/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,6 @@ tasks {
dependsOn("generateJflex")
}

test {
jvmArgs("-Dotel.instrumentation.http.prefer-forwarded-url-scheme=true")
}

check {
dependsOn(testing.suites)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,32 +5,20 @@

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

import io.opentelemetry.instrumentation.api.internal.ConfigPropertiesUtil;
import java.util.Locale;
import java.util.function.Function;
import javax.annotation.Nullable;

final class AlternateUrlSchemeProvider<REQUEST> implements Function<REQUEST, String> {

// if set to true, the instrumentation will prefer the scheme from Forwarded/X-Forwarded-Proto
// headers over the one extracted from the URL
private static final boolean PREFER_FORWARDED_URL_SCHEME =
ConfigPropertiesUtil.getBoolean(
"otel.instrumentation.http.prefer-forwarded-url-scheme", false);
final class ForwardedUrlSchemeProvider<REQUEST> implements Function<REQUEST, String> {

private final HttpServerAttributesGetter<REQUEST, ?> getter;

AlternateUrlSchemeProvider(HttpServerAttributesGetter<REQUEST, ?> getter) {
ForwardedUrlSchemeProvider(HttpServerAttributesGetter<REQUEST, ?> getter) {
this.getter = getter;
}

@Override
public String apply(REQUEST request) {
if (!PREFER_FORWARDED_URL_SCHEME) {
// don't parse headers, extract scheme from the URL
return null;
}

// try Forwarded
for (String forwarded : getter.getHttpRequestHeader(request, "forwarded")) {
String proto = extractProtoFromForwardedHeader(forwarded);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ public AttributesExtractor<REQUEST, RESPONSE> build() {
InternalUrlAttributesExtractor<REQUEST> buildUrlExtractor() {
return new InternalUrlAttributesExtractor<>(
httpAttributesGetter,
new AlternateUrlSchemeProvider<>(httpAttributesGetter),
new ForwardedUrlSchemeProvider<>(httpAttributesGetter),
SemconvStability.emitStableHttpSemconv(),
SemconvStability.emitOldHttpSemconv());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,13 @@
import org.mockito.junit.jupiter.MockitoExtension;

@ExtendWith(MockitoExtension.class)
class AlternateUrlSchemeProviderTest {
class ForwardedUrlSchemeProviderTest {

private static final String REQUEST = "request";

@Mock HttpServerAttributesGetter<String, String> getter;

@InjectMocks AlternateUrlSchemeProvider<String> underTest;
@InjectMocks ForwardedUrlSchemeProvider<String> underTest;

@Test
void noHeaders() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,10 @@ public Integer getServerPort(Map<String, Object> request) {
void normal() {
Map<String, Object> request = new HashMap<>();
request.put("method", "POST");
request.put("urlFull", "http://github.com");
request.put("urlFull", "https://github.com");
request.put("urlPath", "/repositories/1");
request.put("urlQuery", "details=true");
request.put("urlScheme", "http");
request.put("urlScheme", "https");
request.put("header.content-length", "10");
request.put("route", "/repositories/{id}");
request.put("header.user-agent", "okhttp 3.x");
Expand Down Expand Up @@ -159,9 +159,9 @@ void normal() {
entry(SemanticAttributes.SERVER_ADDRESS, "github.com"),
entry(SemanticAttributes.HTTP_METHOD, "POST"),
entry(SemanticAttributes.HTTP_REQUEST_METHOD, "POST"),
entry(SemanticAttributes.HTTP_SCHEME, "http"),
entry(SemanticAttributes.HTTP_SCHEME, "https"),
entry(SemanticAttributes.HTTP_TARGET, "/repositories/1?details=true"),
entry(SemanticAttributes.URL_SCHEME, "http"),
entry(SemanticAttributes.URL_SCHEME, "https"),
entry(SemanticAttributes.URL_PATH, "/repositories/1"),
entry(SemanticAttributes.URL_QUERY, "details=true"),
entry(SemanticAttributes.USER_AGENT_ORIGINAL, "okhttp 3.x"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,10 @@ public String getErrorType(
void normal() {
Map<String, String> request = new HashMap<>();
request.put("method", "POST");
request.put("urlFull", "http://github.com");
request.put("urlFull", "https://github.com");
request.put("urlPath", "/repositories/1");
request.put("urlQuery", "details=true");
request.put("urlScheme", "http");
request.put("urlScheme", "https");
request.put("header.content-length", "10");
request.put("route", "/repositories/{id}");
request.put("header.user-agent", "okhttp 3.x");
Expand Down Expand Up @@ -196,7 +196,7 @@ void normal() {
.containsOnly(
entry(SemanticAttributes.SERVER_ADDRESS, "github.com"),
entry(SemanticAttributes.HTTP_REQUEST_METHOD, "POST"),
entry(SemanticAttributes.URL_SCHEME, "http"),
entry(SemanticAttributes.URL_SCHEME, "https"),
entry(SemanticAttributes.URL_PATH, "/repositories/1"),
entry(SemanticAttributes.URL_QUERY, "details=true"),
entry(SemanticAttributes.USER_AGENT_ORIGINAL, "okhttp 3.x"),
Expand Down Expand Up @@ -417,4 +417,26 @@ void shouldExtractErrorType_other() {

assertThat(attributes.build()).containsEntry(HttpAttributes.ERROR_TYPE, HttpConstants._OTHER);
}

@Test
void shouldPreferUrlSchemeFromForwardedHeader() {
Map<String, String> request = new HashMap<>();
request.put("urlScheme", "http");
request.put("header.forwarded", "proto=https");

Map<String, String> response = new HashMap<>();
response.put("statusCode", "202");

AttributesExtractor<Map<String, String>, Map<String, String>> extractor =
HttpServerAttributesExtractor.create(new TestHttpServerAttributesGetter());

AttributesBuilder startAttributes = Attributes.builder();
extractor.onStart(startAttributes, Context.root(), request);
assertThat(startAttributes.build()).containsOnly(entry(SemanticAttributes.URL_SCHEME, "https"));

AttributesBuilder endAttributes = Attributes.builder();
extractor.onEnd(endAttributes, Context.root(), request, response, null);
assertThat(endAttributes.build())
.containsOnly(entry(SemanticAttributes.HTTP_RESPONSE_STATUS_CODE, 202L));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,6 @@ private static void copyNecessaryConfigToSystemProperties(ConfigProperties confi
for (String property :
asList(
"otel.instrumentation.experimental.span-suppression-strategy",
"otel.instrumentation.http.prefer-forwarded-url-scheme",
"otel.semconv-stability.opt-in")) {
String value = config.getString(property);
if (value != null) {
Expand Down
Loading