Failing streaming endpoints do return trace IDs#45
Failing streaming endpoints do return trace IDs#45bulldozer-bot[bot] merged 3 commits intopalantir:developfrom
Conversation
|
The issue seems to be that the filter is called twice:
|
| @Test | ||
| public void testTraceState_withoutRequestHeadersGeneratesValidTraceResponseHeadersWhenFailing() { | ||
| Response response = target.path("/failing-trace").request().get(); | ||
| assertThat(response.getHeaderString(TraceHttpHeaders.TRACE_ID), not(nullValue())); |
There was a problem hiding this comment.
taking a step back, it's not clear to me that the response needs to contain a trace id header at all (even in the success case).
There was a problem hiding this comment.
In the general case of streaming one? I'm confused, this is how it's been working so far everywhere. If not provided on the request headers, a trace will be created and returned so that you can find out what happens. This makes even more sense IMHO when things go wrong.
For what it's worth, here even forcing the trace with request headers is ignored in the failing streaming case. Doesn't really matter if you force it or let it pick, the exception handling breaks it.
|
I don't know if that best effort else condition I put introduces some bad behaviour in other cases. I suspect it might, otherwise why wouldn't the logic be as simple as that? Perhaps because people don't only use automatic tracing, but the more advanced capabilities. |
| if (maybeSpan.isPresent()) { | ||
| Span span = maybeSpan.get(); | ||
| headers.putSingle(TraceHttpHeaders.TRACE_ID, span.getTraceId()); | ||
| } else { |
There was a problem hiding this comment.
It's pretty funky that this filter is called twice in the case of a failing streaming response... indicates to me that we don't fully understand the jersey behavior here.
ignoring our ignorance for a moment, the fix seems ok to me, but i'd like to see a comment indicating why this else block exists and what it does and how.
| } | ||
|
|
||
| @Test | ||
| public void testTraceState_withoutRequestHeadersGeneratesValidTraceResponseHeadersWhenFailing() { |
There was a problem hiding this comment.
note that this test succeeded before this fix
| } | ||
|
|
||
| @Test | ||
| public void testTraceState_withoutRequestHeadersGeneratesValidTraceResponseHeadersWhenStreaming() { |
There was a problem hiding this comment.
note that this test succeeded before this fix
| } | ||
|
|
||
| @Test | ||
| public void testTraceState_withoutRequestHeadersGeneratesValidTraceResponseHeadersWhenFailingToStream() { |
There was a problem hiding this comment.
this test fails before this fix, and succeeds with this fix
3 participants
Before this PR
A failing streaming endpoint would not return the trace ID as a response header.
After this PR
Currently this PR doesn't fix the issue, it just adds unit tests that show the problem.