Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Run event processors and enrich transactions with contexts #1430

Merged
merged 20 commits into from
Apr 26, 2021

Conversation

marandaneto
Copy link
Contributor

@marandaneto marandaneto commented Apr 21, 2021

📜 Description

Fix: Run event processors and enrich transactions with contexts

💡 Motivation and Context

Event processors were not run for transactions, so contexts were missing.
Some contexts are heavy for Android, hence there's a flag to not do IO nor IPC if it's a transaction.
Spring User is also run, hence transactions for the Spring integration will contain the user info.
Some fields from Event were moved to BaseEvent so transactions contain more data like breadcrumbs, extra, etc...

💚 How did you test it?

running and unit tests

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

@marandaneto marandaneto mentioned this pull request Apr 21, 2021
4 tasks
@marandaneto
Copy link
Contributor Author

@bruno-garcia can we discuss this?

@marandaneto
Copy link
Contributor Author

@bruno-garcia @maciejwalkowiak please review

@marandaneto marandaneto changed the title Fix: Run event processors and enrich transactions Fix: Run event processors and enrich transactions with contexts Apr 22, 2021
@marandaneto marandaneto marked this pull request as ready for review April 22, 2021 12:53
@codecov-commenter
Copy link

codecov-commenter commented Apr 22, 2021

Codecov Report

Merging #1430 (3e9d3bd) into main (6c8aae1) will increase coverage by 0.04%.
The diff coverage is 81.48%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1430      +/-   ##
============================================
+ Coverage     75.84%   75.88%   +0.04%     
- Complexity     1870     1889      +19     
============================================
  Files           186      187       +1     
  Lines          6405     6453      +48     
  Branches        639      633       -6     
============================================
+ Hits           4858     4897      +39     
- Misses         1258     1261       +3     
- Partials        289      295       +6     
Impacted Files Coverage Δ Complexity Δ
.../sentry/DuplicateEventDetectionEventProcessor.java 100.00% <ø> (ø) 11.00 <0.00> (ø)
sentry/src/main/java/io/sentry/SentryEvent.java 67.74% <ø> (-1.15%) 29.00 <0.00> (-11.00)
sentry/src/main/java/io/sentry/EventProcessor.java 50.00% <50.00%> (ø) 1.00 <1.00> (?)
sentry/src/main/java/io/sentry/SentryClient.java 86.30% <74.00%> (-2.15%) 84.00 <8.00> (-1.00)
...entry/src/main/java/io/sentry/SentryBaseEvent.java 78.94% <75.00%> (-6.47%) 36.00 <12.00> (+11.00) ⬇️
...ry/src/main/java/io/sentry/MainEventProcessor.java 84.15% <92.00%> (+8.44%) 41.00 <16.00> (+17.00)
...entry/spring/SentryUserProviderEventProcessor.java 96.55% <100.00%> (+0.55%) 12.00 <3.00> (+2.00)

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 6c8aae1...3e9d3bd. Read the comment docs.

@marandaneto
Copy link
Contributor Author

@maciejwalkowiak could you please check that I didn't break anything in the backend integrations for error monitoring and performance? thanks
Also test that it fixes #1335

@maciejwalkowiak
Copy link
Contributor

As mentioned on Discord, unfortunately event processing for transactions doesn't save us from having a user filter. When transaction is finished, user is cleared from the thread local already.

I'll now just double check if nothing is off with the integrations.

Copy link
Contributor

@maciejwalkowiak maciejwalkowiak left a comment

Choose a reason for hiding this comment

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

Other than these small things mentioned above, LGTM

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