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.

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

Make server.socket.* attributes on the HTTP server side opt-in #8747

Merged
merged 1 commit into from
Jul 19, 2023
Merged

Make server.socket.* attributes on the HTTP server side opt-in #8747

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
3 changes: 2 additions & 1 deletion ...io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ public static <REQUEST, RESPONSE> HttpClientAttributesExtractorBuilder<REQUEST,
addressPortExtractor,
SemconvStability.emitStableHttpSemconv(),
SemconvStability.emitOldHttpSemconv(),
InternalServerAttributesExtractor.Mode.PEER);
InternalServerAttributesExtractor.Mode.PEER,
/* captureServerSocketAttributes= */ true);
this.resendCountIncrementer = resendCountIncrementer;
}

Expand Down
10 changes: 8 additions & 2 deletions ...io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,15 @@ public static <REQUEST, RESPONSE> HttpServerAttributesExtractorBuilder<REQUEST,
NetServerAttributesGetter<REQUEST, RESPONSE> netAttributesGetter,
List<String> capturedRequestHeaders,
List<String> capturedResponseHeaders,
Set<String> knownMethods) {
Set<String> knownMethods,
boolean captureServerSocketAttributes) {
this(
httpAttributesGetter,
netAttributesGetter,
capturedRequestHeaders,
capturedResponseHeaders,
knownMethods,
captureServerSocketAttributes,
HttpRouteHolder::getRoute);
}

Expand All @@ -97,6 +99,7 @@ public static <REQUEST, RESPONSE> HttpServerAttributesExtractorBuilder<REQUEST,
List<String> capturedRequestHeaders,
List<String> capturedResponseHeaders,
Set<String> knownMethods,
boolean captureServerSocketAttributes,
Function<Context, String> httpRouteHolderGetter) {
super(httpAttributesGetter, capturedRequestHeaders, capturedResponseHeaders, knownMethods);
HttpNetAddressPortExtractor<REQUEST> addressPortExtractor =
Expand All @@ -123,7 +126,10 @@ public static <REQUEST, RESPONSE> HttpServerAttributesExtractorBuilder<REQUEST,
addressPortExtractor,
SemconvStability.emitStableHttpSemconv(),
SemconvStability.emitOldHttpSemconv(),
InternalServerAttributesExtractor.Mode.HOST);
InternalServerAttributesExtractor.Mode.HOST,
// we're not capturing these by default, since they're opt-in
// we'll add a configuration flag if someone happens to request them
/* captureServerSocketAttributes= */ false);
internalClientExtractor =
new InternalClientAttributesExtractor<>(
netAttributesGetter,
Expand Down
18 changes: 17 additions & 1 deletion ...telemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ public final class HttpServerAttributesExtractorBuilder<REQUEST, RESPONSE> {
List<String> capturedRequestHeaders = emptyList();
List<String> capturedResponseHeaders = emptyList();
Set<String> knownMethods = HttpConstants.KNOWN_METHODS;
boolean captureServerSocketAttributes = false;

HttpServerAttributesExtractorBuilder(
HttpServerAttributesGetter<REQUEST, RESPONSE> httpAttributesGetter,
Expand Down Expand Up @@ -89,6 +90,20 @@ public HttpServerAttributesExtractorBuilder<REQUEST, RESPONSE> setKnownMethods(
return this;
}

/**
* Configures the extractor to capture the optional {@code server.socket.address} and {@code
* server.socket.port} attributes, which are not collected by default.
*
* @param captureServerSocketAttributes {@code true} if the extractor should collect the optional
* {@code server.socket.address} and {@code server.socket.port} attributes.
*/
@CanIgnoreReturnValue
public HttpServerAttributesExtractorBuilder<REQUEST, RESPONSE> setCaptureServerSocketAttributes(
boolean captureServerSocketAttributes) {
this.captureServerSocketAttributes = captureServerSocketAttributes;
return this;
}
Comment on lines +100 to +105
Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that we don't have to support opt-in things. I'd personally suggest we not provide this configuration unless someone has specific need now, and we can wait to see how your advice PR pans out.

Copy link
Member Author

Choose a reason for hiding this comment

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

spec

Instrumentations SHOULD populate the attribute if and only if the user configures the instrumentation to do so. Instrumentation that doesn't support configuration MUST NOT populate Opt-In attributes.

It's not very explicit about optional support for these; although there is a "SHOULD" here, so it flies, I think. I'm fine with that; and also this is a good subject for the general advice proposed in #3557


/**
* Returns a new {@link HttpServerAttributesExtractor} with the settings of this {@link
* HttpServerAttributesExtractorBuilder}.
Expand All @@ -99,6 +114,7 @@ public AttributesExtractor<REQUEST, RESPONSE> build() {
netAttributesGetter,
capturedRequestHeaders,
capturedResponseHeaders,
knownMethods);
knownMethods,
captureServerSocketAttributes);
}
}
3 changes: 2 additions & 1 deletion ...a/io/opentelemetry/instrumentation/api/instrumenter/net/NetClientAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ private NetClientAttributesExtractor(NetClientAttributesGetter<REQUEST, RESPONSE
FallbackAddressPortExtractor.noop(),
SemconvStability.emitStableHttpSemconv(),
SemconvStability.emitOldHttpSemconv(),
InternalServerAttributesExtractor.Mode.PEER);
InternalServerAttributesExtractor.Mode.PEER,
/* captureServerSocketAttributes= */ true);
}

@Override
Expand Down
3 changes: 2 additions & 1 deletion ...a/io/opentelemetry/instrumentation/api/instrumenter/net/NetServerAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ private NetServerAttributesExtractor(NetServerAttributesGetter<REQUEST, RESPONSE
FallbackAddressPortExtractor.noop(),
SemconvStability.emitStableHttpSemconv(),
SemconvStability.emitOldHttpSemconv(),
InternalServerAttributesExtractor.Mode.HOST);
InternalServerAttributesExtractor.Mode.HOST,
/* captureServerSocketAttributes= */ true);
internalClientExtractor =
new InternalClientAttributesExtractor<>(
getter,
Expand Down
3 changes: 2 additions & 1 deletion .../io/opentelemetry/instrumentation/api/instrumenter/network/ServerAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ public static <REQUEST, RESPONSE> ServerAttributesExtractor<REQUEST, RESPONSE> c
/* emitStableUrlAttributes= */ true,
/* emitOldHttpAttributes= */ false,
// this param does not matter when old semconv is off
InternalServerAttributesExtractor.Mode.HOST);
InternalServerAttributesExtractor.Mode.HOST,
/* captureServerSocketAttributes= */ true);
}

@Override
Expand Down
11 changes: 7 additions & 4 deletions .../instrumentation/api/instrumenter/network/internal/InternalServerAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,23 @@ public final class InternalServerAttributesExtractor<REQUEST, RESPONSE> {
private final boolean emitStableUrlAttributes;
private final boolean emitOldHttpAttributes;
private final Mode oldSemconvMode;
private final boolean captureServerSocketAttributes;

public InternalServerAttributesExtractor(
ServerAttributesGetter<REQUEST, RESPONSE> getter,
BiPredicate<Integer, REQUEST> captureServerPortCondition,
FallbackAddressPortExtractor<REQUEST> fallbackAddressPortExtractor,
boolean emitStableUrlAttributes,
boolean emitOldHttpAttributes,
Mode oldSemconvMode) {
Mode oldSemconvMode,
boolean captureServerSocketAttributes) {
this.getter = getter;
this.captureServerPortCondition = captureServerPortCondition;
this.fallbackAddressPortExtractor = fallbackAddressPortExtractor;
this.emitStableUrlAttributes = emitStableUrlAttributes;
this.emitOldHttpAttributes = emitOldHttpAttributes;
this.oldSemconvMode = oldSemconvMode;
this.captureServerSocketAttributes = captureServerSocketAttributes;
}

public void onStart(AttributesBuilder attributes, REQUEST request) {
Expand Down Expand Up @@ -69,7 +72,7 @@ public void onEnd(AttributesBuilder attributes, REQUEST request, @Nullable RESPO

String serverSocketAddress = getter.getServerSocketAddress(request, response);
if (serverSocketAddress != null && !serverSocketAddress.equals(serverAddressAndPort.address)) {
if (emitStableUrlAttributes) {
if (emitStableUrlAttributes && captureServerSocketAttributes) {
internalSet(attributes, NetworkAttributes.SERVER_SOCKET_ADDRESS, serverSocketAddress);
}
if (emitOldHttpAttributes) {
Expand All @@ -81,7 +84,7 @@ public void onEnd(AttributesBuilder attributes, REQUEST request, @Nullable RESPO
if (serverSocketPort != null
&& serverSocketPort > 0
&& !serverSocketPort.equals(serverAddressAndPort.port)) {
if (emitStableUrlAttributes) {
if (emitStableUrlAttributes && captureServerSocketAttributes) {
internalSet(attributes, NetworkAttributes.SERVER_SOCKET_PORT, (long) serverSocketPort);
}
if (emitOldHttpAttributes) {
Expand All @@ -91,7 +94,7 @@ public void onEnd(AttributesBuilder attributes, REQUEST request, @Nullable RESPO

String serverSocketDomain = getter.getServerSocketDomain(request, response);
if (serverSocketDomain != null && !serverSocketDomain.equals(serverAddressAndPort.address)) {
if (emitStableUrlAttributes) {
if (emitStableUrlAttributes && captureServerSocketAttributes) {
internalSet(attributes, NetworkAttributes.SERVER_SOCKET_DOMAIN, serverSocketDomain);
}
if (emitOldHttpAttributes && oldSemconvMode.socketDomain != null) {
Expand Down
1 change: 1 addition & 0 deletions ...pentelemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ void normal() {
singletonList("Custom-Request-Header"),
singletonList("Custom-Response-Header"),
HttpConstants.KNOWN_METHODS,
false,
routeFromContext);

AttributesBuilder startAttributes = Attributes.builder();
Expand Down
1 change: 1 addition & 0 deletions ...y/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorBothSemconvTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ void normal() {
singletonList("Custom-Request-Header"),
singletonList("Custom-Response-Header"),
HttpConstants.KNOWN_METHODS,
false,
routeFromContext);

AttributesBuilder startAttributes = Attributes.builder();
Expand Down
18 changes: 18 additions & 0 deletions ...instrumentation/api/instrumenter/http/HttpServerAttributesExtractorStableSemconvTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,21 @@ public Integer getServerPort(Map<String, String> request) {
String value = request.get("hostPort");
return value == null ? null : Integer.parseInt(value);
}

@Nullable
@Override
public String getServerSocketAddress(
Map<String, String> request, @Nullable Map<String, String> response) {
return request.get("serverSocketAddress");
}

@Nullable
@Override
public Integer getServerSocketPort(
Map<String, String> request, @Nullable Map<String, String> response) {
String value = request.get("serverSocketPort");
return value == null ? null : Integer.parseInt(value);
}
}

@Test
Expand All @@ -154,6 +169,8 @@ void normal() {
request.put("type", "ipv4");
request.put("protocolName", "http");
request.put("protocolVersion", "2.0");
request.put("serverSocketAddress", "1.2.3.4");
request.put("serverSocketPort", "42");

Map<String, String> response = new HashMap<>();
response.put("statusCode", "202");
Expand All @@ -169,6 +186,7 @@ void normal() {
singletonList("Custom-Request-Header"),
singletonList("Custom-Response-Header"),
HttpConstants.KNOWN_METHODS,
false,
routeFromContext);

AttributesBuilder startAttributes = Attributes.builder();
Expand Down