Port opentelemetry-annotations-1.0 to Instrumenter API #3738
Conversation
|
Looks like |
HaloFour
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
HaloFour
force-pushed
the
withspan-instrumenter
branch
from
beab34c to
c0e1522
Compare
|
/easycla |
1 similar comment
|
/easycla |
...a/io/opentelemetry/instrumentation/api/annotation/support/MethodSpanAttributesExtractor.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| String[] attributeNames = parameterAttributeNamesExtractor.extract(method, parameters); | ||
| if (attributeNames == null || attributeNames.length != parameters.length) { |
There was a problem hiding this comment.
Can attributeNames be null? There are a few more null checks, if you can remove unnecessary ones it's good - we avoid them to make the code easier to read for someone that isn't familiar with it.
There was a problem hiding this comment.
The definition of the functional interface allows for it, null indicating that the method has no parameters to map to attributes: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation-api-annotation-support/src/main/java/io/opentelemetry/instrumentation/api/annotation/support/ParameterAttributeNamesExtractor.java#L26
I could change that to specify returning an empty array if preferred.
There was a problem hiding this comment.
confusingly, I think @Nullable String[] means a non-null array of nullable strings, while String @Nullable [] means a nullable array of non-null strings
There was a problem hiding this comment.
That's terrifying. I do defensively check for either the array being null or each element being null, so would that be @Nullable String @Nullable []?
There was a problem hiding this comment.
I'll review the two implementations to see if it's possible that either will return null, if not I can change the annotation. Is it still worth having the defensive check even if the interface states that it shouldn't be null? I hate the idea of a bug in instrumentation leading to errors at runtime.
There was a problem hiding this comment.
I think @anuraaga's point is that ParameterAttributeNamesExtractor.extract() returns @Nullable String[], so you don't need to check if the array is null
There was a problem hiding this comment.
Got it, so if the array itself can't be null (and that does seem to be the case) then the null check is unnecessary and I can leave the annotations as they are. Thanks!
There was a problem hiding this comment.
we are (slowly) rolling out NullAway checking, which will hopefully help (or maybe add to?) some of the confusion
There was a problem hiding this comment.
(also, I could be wrong about how NullAway interprets @Nullable String[], my assumption is based on how Checker Framework works, it will be good to get confirmation here from @anuraaga)
There was a problem hiding this comment.
I chatted with @anuraaga, NullAway (luckily) does not annotate the contents of the array (like the more powerful, but infinitely more confusing Checker Framework), so @Nullable String[] means what you thought it meant (Nullable applies to the array, not to the strings). Sorry for the confusion.
...c/test/groovy/io/opentelemetry/instrumentation/api/annotation/support/MethodCacheTest.groovy
Outdated
Show resolved
Hide resolved
...ctor-3.1/library/src/main/java/io/opentelemetry/instrumentation/reactor/TracingOperator.java
Show resolved
Hide resolved
...in/java/io/opentelemetry/javaagent/instrumentation/otelannotations/WithSpanInstrumenter.java
Outdated
Show resolved
Hide resolved
Will try to address tonight. Work has been a bit insane this week. 😅 |
HaloFour
force-pushed
the
withspan-instrumenter
branch
from
bb52b2e to
6c36c97
Compare
|
thx ❤️ |
Ports the
opentelemetry-annotations-1.0project to using Instrumenter API in the tracing aspect.Refactors some of the attribute binding code under the
MethodSpanAttributesExtractorclass. Removes the no-longer-usedAsyncSpanEndStrategy/AsyncSpanEndStrategiesimplementations.