Don't report 400 level as error for SERVER spans #4403
Conversation
breedx-splk
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
| @@ -15,7 +16,7 @@ | |||
|
|
|||
| @Override | |||
| public StatusCode extract( | |||
| REQUEST request, @Nullable RESPONSE response, @Nullable Throwable error) { | |||
| REQUEST request, @Nullable RESPONSE response, SpanKind kind, @Nullable Throwable error) { | |||
There was a problem hiding this comment.
Instead of introducing a kind param, how about deciding the behavior basing on the attributes extractor used? I.e. you could check extractor instanceof HttpServerAttributesExtractor and choose the behavior based on that
There was a problem hiding this comment.
+1 to choosing right implementation ASAP. Although it does not make any difference performance-wise.
There was a problem hiding this comment.
Wouldn't that mean that no one could provide their own attributes extractor and have this logic still work? Coupling this to a specific implementation class seems like a bad idea for what we consider to be an API to be used outside of this project.
There was a problem hiding this comment.
HttpSpanStatusExtractor#create() already requires an HttpCommonAttributesExtractor (which cannot be extended directly; you can only extend HttpClientAttributesExtractor or HttpServerAttributesExtractor) so you can't really pass a completely custom attributes extractor.
There was a problem hiding this comment.
But, that would mean that no one could build a kind-oriented attributes extractor (independent of this particular one for http server spans), which still seems like a limitation that might not be desirable.
There was a problem hiding this comment.
would splitting into HttpClientSpanStatusExtractor and HttpServerSpanStatusExtractor work?
we've been finding it useful to split out other extractors based on client/server, e.g. Http(Client|Server)AttributesExtractor and Net(Client|Server)AttributesExtractor (though there's an open discussion on the latter in #4408)
There was a problem hiding this comment.
I considered it but was too concerned it might cause problems with frameworks (and consequently instrumentations) that aren't purely client/server, right? Isn't there instrumentation that can generate both SERVER and CLIENT spans? I'm thinking about messaging systems and http/2 server push, for example.
This was also the thinking behind the "late binding", if you will (I think this is what @iNikem was getting at with the "ASAP" comment).
If this isn't true and we don't think it'll be a problem, we could try that out.
There was a problem hiding this comment.
I considered it but was too concerned it might cause problems with frameworks (and consequently instrumentations) that aren't purely client/server, right? Isn't there instrumentation that can generate both SERVER and CLIENT spans? I'm thinking about messaging systems and http/2 server push, for example.
They already will have to provide the correct SpanKindExtractor. Which means that they will most probably have separate instrumenters per kind. Thus separate SpanStatusExtractor will not be a problem either.
There was a problem hiding this comment.
Isn't there instrumentation that can generate both SERVER and CLIENT spans?
I think all of these are creating two instrumenters already, one to handle server spans and one to handle client spans.
There was a problem hiding this comment.
Ok, let me give that an attempt then.
… create two HttpSpanStatusExtractor.create methods, one for server and one for client.
breedx-splk
force-pushed
the
400_not_server_error
branch
from
917f3d6
to
f94dc85
Compare
...tation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java
Outdated
Show resolved
Hide resolved
...ain/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpSpanStatusExtractor.java
Outdated
Show resolved
Hide resolved
...ain/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpSpanStatusExtractor.java
Outdated
Show resolved
Hide resolved
...test/groovy/io/opentelemetry/instrumentation/api/tracer/HttpServerStatusConverterTest.groovy
Show resolved
Hide resolved
...api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientStatusConverter.java
Show resolved
Hide resolved
| else if (!success && response.status().code() < 500) { | ||
| status UNSET | ||
| } |
There was a problem hiding this comment.
Nit: no need to check for that explicitly; SpanAssert will verify that the status is UNSET in case you haven't done that in your test code
There was a problem hiding this comment.
How/where does SpanAssert enter in? I don't see it.
There was a problem hiding this comment.
Groovy (< insert the Army of Darkness gif >)
Everything under the span(...) {} block executes in the context of SpanAssert (i.e. this is a SpanAssert instance), when the block exits the assertDefaults() function executes.
There was a problem hiding this comment.
@breedx-splk is this another vote for converting groovy tests to java? 😂
There was a problem hiding this comment.
@mateuszrzeszutek Ah, cool, I see it now. It's not great that the status asserts are so far apart...and yeah, @trask I'd love for this not to be groovy...but it sounds like a pretty massive effort...
$ find instrumentation -name '*.groovy' | wc -l
577
$ find instrumentation -name '*.groovy' | xargs cat | wc -l
69566
😱
There was a problem hiding this comment.
it sounds like a pretty massive effort
ya, maybe after instrumentation api stability...
* don't report 400 level as error for server spans * fix HttpServerTest base class * fix JspInstrumentationForward test * split HttpStatusConverter into client and server implementations, and create two HttpSpanStatusExtractor.create methods, one for server and one for client. * rebase * fix test * spotless * fix test * remove unused * use strongly typed attributes converters and rename to overloaded create() * fix tests * remove redundant assert Co-authored-by: Trask Stalnaker <[email protected]>
This resolves #4357
I am assuming that this will likely break some tests, but let's get it going and I'll address those as they arise.