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

Add Request to the Scope. #1270

Merged
merged 4 commits into from
Feb 22, 2021
Merged

Add Request to the Scope. #1270

merged 4 commits into from
Feb 22, 2021

Conversation

maciejwalkowiak
Copy link
Contributor

📜 Description

Add Request to the Scope.

💡 Motivation and Context

We need to have a simple way to pass scope to another thread - including the request. Sample scenario:

HTTP request coming to a server application, this application executes a task on the thread from the pool, Sentry event is sent from this thread. There should be an easy way to pass the HTTP request context to this event.

💚 How did you test it?

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

Refactor Spring and Servlet integrations to set the request on the scope instead of attaching event processor.

@codecov-io
Copy link

codecov-io commented Feb 18, 2021

Codecov Report

Merging #1270 (ab453db) into main (86e9d16) will increase coverage by 0.38%.
The diff coverage is 93.75%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1270      +/-   ##
============================================
+ Coverage     75.16%   75.55%   +0.38%     
- Complexity     1756     1774      +18     
============================================
  Files           183      183              
  Lines          6161     6176      +15     
  Branches        611      613       +2     
============================================
+ Hits           4631     4666      +35     
+ Misses         1252     1231      -21     
- Partials        278      279       +1     
Impacted Files Coverage Δ Complexity Δ
sentry/src/main/java/io/sentry/SentryClient.java 88.04% <50.00%> (-0.28%) 76.00 <0.00> (ø)
sentry/src/main/java/io/sentry/Scope.java 93.03% <100.00%> (+0.21%) 68.00 <2.00> (+3.00)
...ntry/src/main/java/io/sentry/protocol/Request.java 94.11% <100.00%> (+79.30%) 18.00 <3.00> (+15.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 86e9d16...ab453db. Read the comment docs.

public @NotNull Request clone() throws CloneNotSupportedException {
final Request clone = (Request) super.clone();

clone.headers = CollectionUtils.shallowCopy(headers);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think clone.data could be a List or Map and it might require a shallow copy, I guess we've done this in another clone methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean to check if with instanceof? Since data can be anything for now it's just a reference copy.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, but if it's a list/map and items are added/removed, it would mutate both, right, that's what we could avoid.
we did something similar here https://github.com/getsentry/sentry-dart/blob/main/dart/lib/src/protocol/request.dart#L25-L38

Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

LGTM

@bruno-garcia
Copy link
Member

bruno-garcia commented Feb 19, 2021

Regarding the description of the PR (motivation): Passing the scope data to another thread is achieved via cloning the whole hub and passing to another thread, not really just a single scope. The hub has an actual scope stack in it. That's what the unified design specifies at least.

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