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

Jump to bottom

Change a couple of Longs to Integers in Instrumenter API #3043

Merged
merged 4 commits into from
May 20, 2021
Merged

Change a couple of Longs to Integers in Instrumenter API #3043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
70c5d21
peerPort
trask May 19, 2021
eca3a5e
statusCode
trask May 19, 2021
8434b3e
Fix
trask May 19, 2021
b9eeeba
Fix
trask May 19, 2021
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
7 changes: 5 additions & 2 deletions .../java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ protected final void onEnd(AttributesBuilder attributes, REQUEST request, RESPON
attributes,
SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH_UNCOMPRESSED,
requestContentLengthUncompressed(request, response));
set(attributes, SemanticAttributes.HTTP_STATUS_CODE, statusCode(request, response));
Integer statusCode = statusCode(request, response);
if (statusCode != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should use OptionalInt instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's still reasonable chance of someone returning null even with OptionalInt, I guess nullable return has less chance of NPE.

Copy link
Member

Choose a reason for hiding this comment

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

And we're never using any Optionals anywhere in the agent, so even if OptionalInt is a bit nicer and prettier @Nullable Integer is more consistent with the codebase.

set(attributes, SemanticAttributes.HTTP_STATUS_CODE, (long) statusCode);
}
set(attributes, SemanticAttributes.HTTP_FLAVOR, flavor(request, response));
set(
attributes,
Expand Down Expand Up @@ -89,7 +92,7 @@ protected final void onEnd(AttributesBuilder attributes, REQUEST request, RESPON
protected abstract Long requestContentLengthUncompressed(REQUEST request, RESPONSE response);

@Nullable
protected abstract Long statusCode(REQUEST request, RESPONSE response);
protected abstract Integer statusCode(REQUEST request, RESPONSE response);

@Nullable
protected abstract String flavor(REQUEST request, RESPONSE response);
Expand Down
4 changes: 2 additions & 2 deletions .../java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpSpanStatusExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ private HttpSpanStatusExtractor(HttpAttributesExtractor<REQUEST, RESPONSE> attri

@Override
public StatusCode extract(REQUEST request, RESPONSE response, Throwable error) {
Long statusCode = attributesExtractor.statusCode(request, response);
Integer statusCode = attributesExtractor.statusCode(request, response);
if (statusCode != null) {
return HttpStatusConverter.statusFromHttpStatus(statusCode.intValue());
return HttpStatusConverter.statusFromHttpStatus(statusCode);
}
return SpanStatusExtractor.getDefault().extract(request, response, error);
}
Expand Down
4 changes: 2 additions & 2 deletions ...lemetry/instrumentation/api/instrumenter/net/InetSocketAddressNetAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,12 @@ protected final String peerName(REQUEST request, @Nullable RESPONSE response) {

@Override
@Nullable
protected final Long peerPort(REQUEST request, @Nullable RESPONSE response) {
protected final Integer peerPort(REQUEST request, @Nullable RESPONSE response) {
InetSocketAddress address = getAddress(request, response);
if (address == null) {
return null;
}
return (long) address.getPort();
return address.getPort();
}

@Override
Expand Down
12 changes: 9 additions & 3 deletions ...in/java/io/opentelemetry/instrumentation/api/instrumenter/net/NetAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,20 @@ protected final void onStart(AttributesBuilder attributes, REQUEST request) {
set(attributes, SemanticAttributes.NET_TRANSPORT, transport(request));
set(attributes, SemanticAttributes.NET_PEER_IP, peerIp(request, null));
set(attributes, SemanticAttributes.NET_PEER_NAME, peerName(request, null));
set(attributes, SemanticAttributes.NET_PEER_PORT, peerPort(request, null));
Integer peerPort = peerPort(request, null);
if (peerPort != null) {
set(attributes, SemanticAttributes.NET_PEER_PORT, (long) peerPort);
}
}

@Override
protected final void onEnd(AttributesBuilder attributes, REQUEST request, RESPONSE response) {
set(attributes, SemanticAttributes.NET_PEER_IP, peerIp(request, response));
set(attributes, SemanticAttributes.NET_PEER_NAME, peerName(request, response));
set(attributes, SemanticAttributes.NET_PEER_PORT, peerPort(request, response));
Integer peerPort = peerPort(request, response);
if (peerPort != null) {
set(attributes, SemanticAttributes.NET_PEER_PORT, (long) peerPort);
}
}

@Nullable
Expand All @@ -51,7 +57,7 @@ protected final void onEnd(AttributesBuilder attributes, REQUEST request, RESPON
* phases of processing.
*/
@Nullable
protected abstract Long peerPort(REQUEST request, @Nullable RESPONSE response);
protected abstract Integer peerPort(REQUEST request, @Nullable RESPONSE response);

/**
* This method will be called twice: both when the request starts ({@code response} is always null
Expand Down
4 changes: 2 additions & 2 deletions ...a/io/opentelemetry/instrumentation/api/instrumenter/http/HttpAttributesExtractorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ protected Long requestContentLengthUncompressed(
}

@Override
protected Long statusCode(Map<String, String> request, Map<String, String> response) {
return Long.parseLong(response.get("statusCode"));
protected Integer statusCode(Map<String, String> request, Map<String, String> response) {
return Integer.parseInt(response.get("statusCode"));
}

@Override
Expand Down
12 changes: 6 additions & 6 deletions ...a/io/opentelemetry/instrumentation/api/instrumenter/http/HttpSpanStatusExtractorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,27 +25,27 @@ class HttpSpanStatusExtractorTest {
@Mock private HttpAttributesExtractor<Map<String, String>, Map<String, String>> extractor;

@ParameterizedTest
@ValueSource(longs = {1, 100, 101, 200, 201, 300, 301, 400, 401, 500, 501, 600, 601})
void hasStatus(long statusCode) {
@ValueSource(ints = {1, 100, 101, 200, 201, 300, 301, 400, 401, 500, 501, 600, 601})
void hasStatus(int statusCode) {
when(extractor.statusCode(anyMap(), anyMap())).thenReturn(statusCode);

assertThat(
HttpSpanStatusExtractor.create(extractor)
.extract(Collections.emptyMap(), Collections.emptyMap(), null))
.isEqualTo(HttpStatusConverter.statusFromHttpStatus((int) statusCode));
.isEqualTo(HttpStatusConverter.statusFromHttpStatus(statusCode));
}

@ParameterizedTest
@ValueSource(longs = {1, 100, 101, 200, 201, 300, 301, 400, 401, 500, 501, 600, 601})
void hasStatus_ignoresException(long statusCode) {
@ValueSource(ints = {1, 100, 101, 200, 201, 300, 301, 400, 401, 500, 501, 600, 601})
void hasStatus_ignoresException(int statusCode) {
when(extractor.statusCode(anyMap(), anyMap())).thenReturn(statusCode);

// Presence of exception has no effect.
assertThat(
HttpSpanStatusExtractor.create(extractor)
.extract(
Collections.emptyMap(), Collections.emptyMap(), new IllegalStateException()))
.isEqualTo(HttpStatusConverter.statusFromHttpStatus((int) statusCode));
.isEqualTo(HttpStatusConverter.statusFromHttpStatus(statusCode));
}

@Test
Expand Down
6 changes: 3 additions & 3 deletions ...ava/io/opentelemetry/instrumentation/api/instrumenter/net/NetAttributesExtractorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,11 @@ protected String peerName(Map<String, String> request, Map<String, String> respo
}

@Override
protected Long peerPort(Map<String, String> request, Map<String, String> response) {
protected Integer peerPort(Map<String, String> request, Map<String, String> response) {
if (response != null) {
return Long.valueOf(response.get("peerPort"));
return Integer.valueOf(response.get("peerPort"));
}
return Long.valueOf(request.get("peerPort"));
return Integer.valueOf(request.get("peerPort"));
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions ...aagent/instrumentation/apachehttpclient/v2_0/ApacheHttpClientHttpAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,9 @@ protected Long requestContentLengthUncompressed(HttpMethod httpMethod, Void unus

@Override
@Nullable
protected Long statusCode(HttpMethod httpMethod, Void unused) {
protected Integer statusCode(HttpMethod httpMethod, Void unused) {
StatusLine statusLine = httpMethod.getStatusLine();
return statusLine == null ? null : (long) statusLine.getStatusCode();
return statusLine == null ? null : statusLine.getStatusCode();
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions ...vaagent/instrumentation/apachehttpclient/v2_0/ApacheHttpClientNetAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ protected String transport(HttpMethod httpMethod) {
}

@Override
protected @Nullable Long peerPort(HttpMethod httpMethod, @Nullable Void unused) {
protected @Nullable Integer peerPort(HttpMethod httpMethod, @Nullable Void unused) {
HostConfiguration hostConfiguration = httpMethod.getHostConfiguration();
return hostConfiguration != null ? (long) hostConfiguration.getPort() : null;
return hostConfiguration != null ? hostConfiguration.getPort() : null;
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions ...in/java/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaHttpAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,10 @@ protected Long requestContentLengthUncompressed(RequestContext ctx, RequestLog r

@Override
@Nullable
protected Long statusCode(RequestContext ctx, RequestLog requestLog) {
protected Integer statusCode(RequestContext ctx, RequestLog requestLog) {
HttpStatus status = requestLog.responseHeaders().status();
if (!status.equals(HttpStatus.UNKNOWN)) {
return (long) status.code();
return status.code();
}
return null;
}
Expand Down
5 changes: 2 additions & 3 deletions ...main/java/io/opentelemetry/javaagent/instrumentation/jdbc/JdbcNetAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,8 @@ protected String peerName(DbRequest request, @Nullable Void response) {

@Nullable
@Override
protected Long peerPort(DbRequest request, @Nullable Void response) {
Integer port = request.getDbInfo().getPort();
return port == null ? null : port.longValue();
protected Integer peerPort(DbRequest request, @Nullable Void response) {
return request.getDbInfo().getPort();
}

@Nullable
Expand Down