Update OTel snapshot to 723 for recordException #786
Conversation
anuraaga
requested review from
iNikem,
jkwatson,
trask and
tylerbenson
as code owners
| @@ -287,6 +288,32 @@ class TracerTest extends AgentTestRunner { | |||
| } | |||
| } | |||
|
|
|||
| def "capture exception()"() { | |||
|
Do you want also to fix all Tracers to use new API for exceptions? |
|
I'll do it in a separate PR I just want to get the baseline up on this one. There seems to be something wrong with async servlet which I'm digging |
|
I have spent 1.5 days digging into async servlets :( Please wait for me to file a bug with my findings to not duplicate my efforts. |
|
A, you have another issue. Mine is here: #787 |
|
@trask Would you be able to take a look at this branch? Having trouble getting servlet async tests passing. From what I can tell, previously the assertions weren't correct since they were effectively |
|
oh, you're right, that test validation was not correct, good find! i sent a PR to your branch anuraaga#2, i couldn't get rid of the |
|
Ok - yeah if nothing is obvious it's good to get this baseline update in and we'll debug more separately. |
* spotless * fix tests
| TRACER.end(span, response.getStatus()); | ||
| } | ||
| } | ||
|
|
||
| public static void contentLengthHelper(Span span, ServletResponse response) { | ||
| if (response instanceof CountingHttpServletResponse) { |
There was a problem hiding this comment.
Notice JettyHandler didn't seem to have implemented counting, so thought it'd be a quick fix to add it, but some conflict between unnamed classloader and app classloader here :( Tomorrow will give another small look and then revert this and remove the assertion for now.
There was a problem hiding this comment.
Found it was because I was missing this advice from the helper classes (was referring to servlet-3.0 while writing this). Filed #796 since this was confusing.
…-instr-java into update-otel-snapshot-723
This doesn't update to the latest snapshot which would have lots of conflicts with #785