Skip to content

Conversation

@felixbarny
Copy link
Member

@felixbarny felixbarny commented Jun 29, 2018

Previously, most of the transaction context was set after the execution of a request.
When capturing an exception, the transaction context is copied over at its current state.
That means when the user captures an exception, the transaction context is not yet
populated and therefore, the Error context is empty.

This changes the initialization of the transaction context so that it is set
before the request starts.

That way, we can be sure that the Error context is always set

Previously, most of the transaction context was set after the execution of a request.
When capturing an exception, the transaction context is copied over at it's current state.
That means when the user captures an exception, the the transaction context is not yet
populated and therefore, the Error context is empty.

This changes the initialization of the transaction context so that it is set
before the request starts.

That way, we can be sure that the Error context is always set
@felixbarny
Copy link
Member Author

Hey @beniwohli, could you review the comments in the commit message and in ServletTransactionHelper? I think the approach outlined in the comments should work, but I'd love to hear your opinion about it.

@felixbarny felixbarny requested a review from beniwohli June 29, 2018 15:18
@codecov-io
Copy link

codecov-io commented Jun 29, 2018

Codecov Report

Merging #114 into master will increase coverage by 0.34%.
The diff coverage is 37.77%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #114      +/-   ##
============================================
+ Coverage     76.73%   77.08%   +0.34%     
- Complexity      947      953       +6     
============================================
  Files            99       99              
  Lines          3280     3282       +2     
  Branches        291      289       -2     
============================================
+ Hits           2517     2530      +13     
+ Misses          602      600       -2     
+ Partials        161      152       -9
Impacted Files Coverage Δ Complexity Δ
.../java/co/elastic/apm/servlet/ServletApiAdvice.java 5.26% <0%> (-0.46%) 1 <0> (ø)
...a/co/elastic/apm/impl/transaction/Transaction.java 76.74% <100%> (+0.55%) 26 <1> (+1) ⬆️
...lastic/apm/report/serialize/DslJsonSerializer.java 84.61% <100%> (+0.21%) 84 <2> (+1) ⬆️
...ain/java/co/elastic/apm/impl/ElasticApmTracer.java 78.14% <100%> (+0.14%) 41 <0> (ø) ⬇️
.../elastic/apm/servlet/ServletTransactionHelper.java 84.33% <75%> (+5.58%) 26 <5> (+4) ⬆️
.../java/co/elastic/apm/report/ApmServerReporter.java 54.65% <0%> (+0.1%) 11% <0%> (ø) ⬇️
...elastic/apm/report/ApmServerHttpPayloadSender.java 76.54% <0%> (+2.73%) 7% <0%> (-1%) ⬇️
...a/co/elastic/apm/report/ReportingEventHandler.java 93.54% <0%> (+11.73%) 10% <0%> (ø) ⬇️
...o/elastic/apm/impl/error/TransactionReference.java 66.66% <0%> (+16.66%) 4% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c2c780...5a49986. Read the comment docs.

Copy link

@beniwohli beniwohli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixbarny looks good! It's still not quite the same behavior as the Python (and AFAIK most other) agent, but close enough 👍

@felixbarny
Copy link
Member Author

Thanks for having a look :)

@felixbarny felixbarny merged commit a6b3758 into elastic:master Jul 2, 2018
@felixbarny felixbarny deleted the error-context-overhaul branch July 2, 2018 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants