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

Remove Attributes combination from onEnd #11959

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
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
@@ -73,9 +73,7 @@ public void onEnd(Context context, Attributes endAttributes, long endNanos) {
return;
}
clientDurationHistogram.record(
(endNanos - state.startTimeNanos()) / NANOS_PER_MS,
state.startAttributes().toBuilder().putAll(endAttributes).build(),
context);
(endNanos - state.startTimeNanos()) / NANOS_PER_MS, endAttributes, context);
}

@AutoValue
Original file line number Diff line number Diff line change
@@ -28,11 +28,7 @@ abstract class RpcCommonAttributesExtractor<REQUEST, RESPONSE>
}

@Override
public final void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
internalSet(attributes, RPC_SYSTEM, getter.getSystem(request));
internalSet(attributes, RPC_SERVICE, getter.getService(request));
internalSet(attributes, RPC_METHOD, getter.getMethod(request));
}
public final void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {}

@Override
public final void onEnd(
@@ -42,5 +38,8 @@ public final void onEnd(
@Nullable RESPONSE response,
@Nullable Throwable error) {
// No response attributes
internalSet(attributes, RPC_SYSTEM, getter.getSystem(request));
internalSet(attributes, RPC_SERVICE, getter.getService(request));
internalSet(attributes, RPC_METHOD, getter.getMethod(request));
}
}
Original file line number Diff line number Diff line change
@@ -73,9 +73,7 @@ public void onEnd(Context context, Attributes endAttributes, long endNanos) {
return;
}
serverDurationHistogram.record(
(endNanos - state.startTimeNanos()) / NANOS_PER_MS,
state.startAttributes().toBuilder().putAll(endAttributes).build(),
context);
(endNanos - state.startTimeNanos()) / NANOS_PER_MS, endAttributes, context);
}

@AutoValue
Original file line number Diff line number Diff line change
@@ -250,6 +250,7 @@ private void doEnd(
if (operationListeners.length != 0) {
long endNanos = getNanos(endTime);
for (int i = operationListeners.length - 1; i >= 0; i--) {

Copy link
Contributor

Choose a reason for hiding this comment

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

The modification here is useless.

operationListeners[i].onEnd(context, attributes, endNanos);
}
}
Original file line number Diff line number Diff line change
@@ -70,16 +70,6 @@ public static <REQUEST, RESPONSE> HttpClientAttributesExtractorBuilder<REQUEST,
@Override
public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
super.onStart(attributes, parentContext, request);

internalServerExtractor.onStart(attributes, request);

String fullUrl = stripSensitiveData(getter.getUrlFull(request));
internalSet(attributes, UrlAttributes.URL_FULL, fullUrl);

int resendCount = resendCountIncrementer.applyAsInt(parentContext);
if (resendCount > 0) {
attributes.put(HttpAttributes.HTTP_REQUEST_RESEND_COUNT, resendCount);
}
}

@Override
@@ -90,7 +80,15 @@ public void onEnd(
@Nullable RESPONSE response,
@Nullable Throwable error) {
super.onEnd(attributes, context, request, response, error);
internalServerExtractor.onEnd(attributes, request);

String fullUrl = stripSensitiveData(getter.getUrlFull(request));
internalSet(attributes, UrlAttributes.URL_FULL, fullUrl);

int resendCount = resendCountIncrementer.applyAsInt(context);
if (resendCount > 0) {
attributes.put(HttpAttributes.HTTP_REQUEST_RESEND_COUNT, resendCount);
}
internalNetworkExtractor.onEnd(attributes, request, response);
}

Original file line number Diff line number Diff line change
@@ -78,9 +78,7 @@ public void onEnd(Context context, Attributes endAttributes, long endNanos) {
return;
}

Attributes attributes = state.startAttributes().toBuilder().putAll(endAttributes).build();

duration.record((endNanos - state.startTimeNanos()) / NANOS_PER_S, attributes, context);
duration.record((endNanos - state.startTimeNanos()) / NANOS_PER_S, endAttributes, context);
}

@AutoValue
Original file line number Diff line number Diff line change
@@ -57,7 +57,15 @@ abstract class HttpCommonAttributesExtractor<
}

@Override
public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {}

@Override
public void onEnd(
AttributesBuilder attributes,
Context context,
REQUEST request,
@Nullable RESPONSE response,
@Nullable Throwable error) {
String method = getter.getHttpRequestMethod(request);
if (method == null || knownMethods.contains(method)) {
internalSet(attributes, HttpAttributes.HTTP_REQUEST_METHOD, method);
@@ -72,15 +80,6 @@ public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST
internalSet(attributes, requestAttributeKey(name), values);
}
}
}

@Override
public void onEnd(
AttributesBuilder attributes,
Context context,
REQUEST request,
@Nullable RESPONSE response,
@Nullable Throwable error) {

Integer statusCode = null;
if (response != null) {
Original file line number Diff line number Diff line change
@@ -76,13 +76,6 @@ public static <REQUEST, RESPONSE> HttpServerAttributesExtractorBuilder<REQUEST,
@Override
public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
super.onStart(attributes, parentContext, request);
Copy link
Contributor

Choose a reason for hiding this comment

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

Only the attributes that are added at the start will be available for sampling. Moving all the attributes to the onEnd will make sampling impossible.

Copy link
Contributor Author

@wgy035 wgy035 Aug 7, 2024

Choose a reason for hiding this comment

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

So I need to figure out which attributes impact sampling, and those attributes are the minimum set need to retain, right?

According to semantic-conventions(e.g. http-spans), I'd like to extract the unique attributes of span at the start without needing to pass them on. The common attributes attributes should be extracted at the end.


internalUrlExtractor.onStart(attributes, request);
internalServerExtractor.onStart(attributes, request);
internalClientExtractor.onStart(attributes, request);

internalSet(attributes, HttpAttributes.HTTP_ROUTE, getter.getHttpRoute(request));
internalSet(attributes, UserAgentAttributes.USER_AGENT_ORIGINAL, userAgent(request));
}

@Override
@@ -94,7 +87,13 @@ public void onEnd(
@Nullable Throwable error) {

super.onEnd(attributes, context, request, response, error);
internalSet(attributes, UserAgentAttributes.USER_AGENT_ORIGINAL, userAgent(request));

internalUrlExtractor.onEnd(attributes, request);
internalServerExtractor.onEnd(attributes, request);
internalClientExtractor.onEnd(attributes, request);

internalSet(attributes, HttpAttributes.HTTP_ROUTE, getter.getHttpRoute(request));
internalNetworkExtractor.onEnd(attributes, request, response);

internalSet(attributes, HttpAttributes.HTTP_ROUTE, httpRouteGetter.apply(context));
Original file line number Diff line number Diff line change
@@ -78,9 +78,7 @@ public void onEnd(Context context, Attributes endAttributes, long endNanos) {
return;
}

Attributes attributes = state.startAttributes().toBuilder().putAll(endAttributes).build();

duration.record((endNanos - state.startTimeNanos()) / NANOS_PER_S, attributes, context);
duration.record((endNanos - state.startTimeNanos()) / NANOS_PER_S, endAttributes, context);
}

@AutoValue
Original file line number Diff line number Diff line change
@@ -44,15 +44,15 @@ public static <REQUEST, RESPONSE> ClientAttributesExtractor<REQUEST, RESPONSE> c
}

@Override
public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
internalExtractor.onStart(attributes, request);
}
public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {}

@Override
public void onEnd(
AttributesBuilder attributes,
Context context,
REQUEST request,
@Nullable RESPONSE response,
@Nullable Throwable error) {}
@Nullable Throwable error) {
internalExtractor.onEnd(attributes, request);
}
}
Original file line number Diff line number Diff line change
@@ -43,15 +43,15 @@ public static <REQUEST, RESPONSE> ServerAttributesExtractor<REQUEST, RESPONSE> c
}

@Override
public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
internalExtractor.onStart(attributes, request);
}
public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {}

@Override
public void onEnd(
AttributesBuilder attributes,
Context context,
REQUEST request,
@Nullable RESPONSE response,
@Nullable Throwable error) {}
@Nullable Throwable error) {
internalExtractor.onEnd(attributes, request);
}
}
Original file line number Diff line number Diff line change
@@ -25,7 +25,7 @@ public InternalClientAttributesExtractor(
this.capturePort = capturePort;
}

public void onStart(AttributesBuilder attributes, REQUEST request) {
public void onEnd(AttributesBuilder attributes, REQUEST request) {
AddressAndPort clientAddressAndPort = addressAndPortExtractor.extract(request);

if (clientAddressAndPort.address != null) {
Original file line number Diff line number Diff line change
@@ -23,7 +23,7 @@ public InternalServerAttributesExtractor(
this.addressAndPortExtractor = addressAndPortExtractor;
}

public void onStart(AttributesBuilder attributes, REQUEST request) {
public void onEnd(AttributesBuilder attributes, REQUEST request) {
AddressAndPort serverAddressAndPort = addressAndPortExtractor.extract(request);

if (serverAddressAndPort.address != null) {
Original file line number Diff line number Diff line change
@@ -42,15 +42,15 @@ public static <REQUEST, RESPONSE> UrlAttributesExtractor<REQUEST, RESPONSE> crea
}

@Override
public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
internalExtractor.onStart(attributes, request);
}
public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {}

@Override
public void onEnd(
AttributesBuilder attributes,
Context context,
REQUEST request,
@Nullable RESPONSE response,
@Nullable Throwable error) {}
@Nullable Throwable error) {
internalExtractor.onEnd(attributes, request);
}
}
Original file line number Diff line number Diff line change
@@ -27,7 +27,7 @@ public InternalUrlAttributesExtractor(
this.alternateSchemeProvider = alternateSchemeProvider;
}

public void onStart(AttributesBuilder attributes, REQUEST request) {
public void onEnd(AttributesBuilder attributes, REQUEST request) {
String urlScheme = getUrlScheme(request);
String urlPath = getter.getUrlPath(request);
String urlQuery = getter.getUrlQuery(request);
Original file line number Diff line number Diff line change
@@ -169,28 +169,26 @@ void normal() {

AttributesBuilder startAttributes = Attributes.builder();
extractor.onStart(startAttributes, Context.root(), request);
assertThat(startAttributes.build())
.containsOnly(
entry(HttpAttributes.HTTP_REQUEST_METHOD, "POST"),
entry(UrlAttributes.URL_FULL, "http://github.com"),
entry(
AttributeKey.stringArrayKey("http.request.header.custom-request-header"),
asList("123", "456")),
entry(ServerAttributes.SERVER_ADDRESS, "github.com"),
entry(ServerAttributes.SERVER_PORT, 80L),
entry(HttpAttributes.HTTP_REQUEST_RESEND_COUNT, 2L));

AttributesBuilder endAttributes = Attributes.builder();
extractor.onEnd(endAttributes, Context.root(), request, response, null);
assertThat(endAttributes.build())
.containsOnly(
entry(HttpAttributes.HTTP_RESPONSE_STATUS_CODE, 202L),
entry(
AttributeKey.stringArrayKey("http.response.header.custom-response-header"),
asList("654", "321")),
entry(NetworkAttributes.NETWORK_PROTOCOL_VERSION, "1.1"),
entry(NetworkAttributes.NETWORK_PEER_ADDRESS, "4.3.2.1"),
entry(NetworkAttributes.NETWORK_PEER_PORT, 456L));
.containsOnly(
entry(HttpAttributes.HTTP_RESPONSE_STATUS_CODE, 202L),
entry(
AttributeKey.stringArrayKey("http.response.header.custom-response-header"),
asList("654", "321")),
entry(NetworkAttributes.NETWORK_PROTOCOL_VERSION, "1.1"),
entry(NetworkAttributes.NETWORK_PEER_ADDRESS, "4.3.2.1"),
entry(HttpAttributes.HTTP_REQUEST_METHOD, "POST"),
entry(UrlAttributes.URL_FULL, "http://github.com"),
entry(
AttributeKey.stringArrayKey("http.request.header.custom-request-header"),
asList("123", "456")),
entry(ServerAttributes.SERVER_ADDRESS, "github.com"),
entry(ServerAttributes.SERVER_PORT, 80L),
entry(HttpAttributes.HTTP_REQUEST_RESEND_COUNT, 2L),
entry(NetworkAttributes.NETWORK_PEER_PORT, 456L));
}

@ParameterizedTest
@@ -354,19 +352,17 @@ void shouldExtractServerAddressAndPortFromHostHeader() {
response.put("statusCode", "200");

AttributesExtractor<Map<String, String>, Map<String, String>> extractor =
HttpClientAttributesExtractor.create(new TestHttpClientAttributesGetter());
HttpClientAttributesExtractor.create(new TestHttpClientAttributesGetter());

AttributesBuilder startAttributes = Attributes.builder();
extractor.onStart(startAttributes, Context.root(), request);
assertThat(startAttributes.build())
.containsOnly(
entry(ServerAttributes.SERVER_ADDRESS, "github.com"),
entry(ServerAttributes.SERVER_PORT, 123L));

AttributesBuilder endAttributes = Attributes.builder();
extractor.onEnd(endAttributes, Context.root(), request, response, null);
assertThat(endAttributes.build())
.containsOnly(entry(HttpAttributes.HTTP_RESPONSE_STATUS_CODE, 200L));
.containsOnly(entry(HttpAttributes.HTTP_RESPONSE_STATUS_CODE, 200L),
entry(ServerAttributes.SERVER_ADDRESS, "github.com"),
entry(ServerAttributes.SERVER_PORT, 123L));
}

@Test
@@ -381,22 +377,20 @@ void shouldExtractPeerAddressEvenIfItDuplicatesServerAddress() {
response.put("statusCode", "200");

AttributesExtractor<Map<String, String>, Map<String, String>> extractor =
HttpClientAttributesExtractor.create(new TestHttpClientAttributesGetter());
HttpClientAttributesExtractor.create(new TestHttpClientAttributesGetter());

AttributesBuilder startAttributes = Attributes.builder();
extractor.onStart(startAttributes, Context.root(), request);
assertThat(startAttributes.build())
.containsOnly(
entry(ServerAttributes.SERVER_ADDRESS, "1.2.3.4"),
entry(ServerAttributes.SERVER_PORT, 123L));

AttributesBuilder endAttributes = Attributes.builder();
extractor.onEnd(endAttributes, Context.root(), request, response, null);
assertThat(endAttributes.build())
.containsOnly(
entry(HttpAttributes.HTTP_RESPONSE_STATUS_CODE, 200L),
entry(NetworkAttributes.NETWORK_PEER_ADDRESS, "1.2.3.4"),
entry(NetworkAttributes.NETWORK_PEER_PORT, 456L));
.containsOnly(
entry(HttpAttributes.HTTP_RESPONSE_STATUS_CODE, 200L),
entry(NetworkAttributes.NETWORK_PEER_ADDRESS, "1.2.3.4"),
entry(NetworkAttributes.NETWORK_PEER_PORT, 456L),
entry(ServerAttributes.SERVER_ADDRESS, "1.2.3.4"),
entry(ServerAttributes.SERVER_PORT, 123L));
}

@Test
Original file line number Diff line number Diff line change
@@ -51,7 +51,10 @@ void collectsMetrics() {

Attributes responseAttributes =
Attributes.builder()
.put(HttpAttributes.HTTP_RESPONSE_STATUS_CODE, 200)
.put(HttpAttributes.HTTP_REQUEST_METHOD, "GET")
.put(ServerAttributes.SERVER_PORT, 1234)
.put(ServerAttributes.SERVER_ADDRESS, "localhost")
.put(HttpAttributes.HTTP_RESPONSE_STATUS_CODE, 200)
.put(ErrorAttributes.ERROR_TYPE, "400")
.put(HttpIncubatingAttributes.HTTP_REQUEST_BODY_SIZE, 100)
.put(HttpIncubatingAttributes.HTTP_RESPONSE_BODY_SIZE, 200)
Loading