-
Notifications
You must be signed in to change notification settings - Fork 845
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
Change a couple of Longs to Integers in Instrumenter API #3043
Conversation
@@ -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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 Optional
s anywhere in the agent, so even if OptionalInt
is a bit nicer and prettier @Nullable Integer
is more consistent with the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it adds grokkability for the type to match the attribute type, it does seem to significantly improve the instrumentation code
@@ -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) { |
There was a problem hiding this comment.
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.
These two methods feel like they should return
Integer
, and it also makes implementing them a bit easier: