-
Notifications
You must be signed in to change notification settings - Fork 986
add Nullaway to instrumentation API #14458
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
base: main
Are you sure you want to change the base?
Conversation
🔧 The result from spotlessApply was committed to the PR branch. |
1 similar comment
🔧 The result from spotlessApply was committed to the PR branch. |
...o/opentelemetry/javaagent/instrumentation/activejhttp/ActivejHttpServerAttributesGetter.java
Outdated
Show resolved
Hide resolved
4d1d120
to
dca4806
Compare
private static final Instrumenter<Void, Void> INSTRUMENTER = | ||
Instrumenter.<Void, Void>builder( | ||
private static final Object REQUEST = new Object(); | ||
|
||
private static final Instrumenter<Object, Void> INSTRUMENTER = | ||
Instrumenter.<Object, Void>builder( |
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.
why were these changes needed?
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.
it passed null
as request param - which is not nullable
@Nullable String method, | ||
@Nullable String route, | ||
int updatedBySourceOrder, | ||
@Nullable Span span) { |
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.
just checking, is null
Span ever passed in here?
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.
just above: create(method, route, updatedBySourceOrder, null);
@@ -19,6 +19,7 @@ | |||
* This class is internal and is hence not for public use. Its APIs are unstable and can change at | |||
* any time. | |||
*/ | |||
@SuppressWarnings("NullAway") // setters called in static initializer |
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 don't see static initializer in this class? or is it called from another class?
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.
It's the static methods setInstrumenterAccess
and setInstrumenterBuilderAccess
I realized that we could also add an @Initializer
annotation to those methods - but we'd need a place to add this annotation.
See https://github.com/uber/NullAway/wiki/Supported-Annotations#initialization
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.
oh, maybe it wants the static fields to be annotated with @Nullable
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.
if we'd do that - we'd get an error when the field is accessed: instrumenterAccess.startAndEnd
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 just added the annotation to see if that looks better
@trask can you check again? |
No description provided.