Skip to content

fix: Don't create transactions for HTTP Requests #1237

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

Merged
merged 13 commits into from
Jul 26, 2021

Conversation

brustolin
Copy link
Contributor

@brustolin brustolin commented Jul 22, 2021

📜 Description

Prevent SentryNetworkTracker from creating a transaction if there is no active transaction in SentryPerformanceTracker.

💡 Motivation and Context

There should not be standalone transactions for HTTP requests

Fix #1216

💚 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
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

Sorry, something went wrong.

@brustolin
Copy link
Contributor Author

Right now, SentryNetworkTracker only works with SentryUIPerformanceTracker.
What if there is no UI Tracking happening but there is a span in the scope?
Should we automatically track any request happening while there is a user span in the scope?

@brustolin brustolin changed the title fix: Don't create transactions for HTTP Requests. fix: Don't create transactions for HTTP Requests Jul 22, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2021

Codecov Report

Merging #1237 (92f4123) into master (c8e76e2) will increase coverage by 0.18%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1237      +/-   ##
==========================================
+ Coverage   95.37%   95.55%   +0.18%     
==========================================
  Files         148      148              
  Lines        5206     5215       +9     
==========================================
+ Hits         4965     4983      +18     
+ Misses        241      232       -9     
Impacted Files Coverage Δ
Sources/Sentry/SentryNetworkTracker.m 100.00% <100.00%> (+4.65%) ⬆️
Sources/Sentry/SentryPerformanceTracker.m 98.48% <100.00%> (+0.02%) ⬆️
Sources/Sentry/SentryCrashIntegration.m 100.00% <0.00%> (ø)
Sources/Sentry/SentryClient.m 95.61% <0.00%> (+0.01%) ⬆️
Sources/Sentry/SentryNSURLRequest.m 75.00% <0.00%> (+3.57%) ⬆️
Sources/Sentry/SentryHttpInterceptor.m 96.00% <0.00%> (+6.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 c8e76e2...92f4123. Read the comment docs.

@philipphofmann
Copy link
Member

philipphofmann commented Jul 22, 2021

Right now, SentryNetworkTracker only works with SentryUIPerformanceTracker.
What if there is no UI Tracking happening but there is a span in the scope?
Should we automatically track any request happening while there is a user span in the scope?

Yes, I think if there is a span on the scope and autoPerformanceTracking is enabled we should add the HTTP span. At least we do that on Android. Can you confirm this @bruno-garcia?

@brustolin
Copy link
Contributor Author

brustolin commented Jul 22, 2021

Yes, I think if there is a span on the scope and autoPerformanceTracking is enabled we should add the HTTP span. At least we do that on Android. Can you confirm this @bruno-garcia?

So that's raise the question: if there is a UI transaction going on, but it is not in the scope because the user has attached something in the scope first. Which one should we use to create the request span, the UI transaction or the one in the scope?

If we take Android in consideration, since they use only the scope to manage performance tracking, this request span should be added to the one in the scope right?

@bruno-garcia
Copy link
Member

I believe we should add the http span created as a child of whatever transaction is bound to the scope (and not any of its spans).

@philipphofmann
Copy link
Member

I believe we should add the http span created as a child of whatever transaction is bound to the scope (and not any of its spans).

I agreee. I think this makes sense. We can start with that because it's simple and we can do more complex stuff later.

@philipphofmann philipphofmann force-pushed the fix/dontcreatetransactionforrequest branch from a820d79 to 2d64527 Compare July 23, 2021 09:56
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Thanks, we should add a few more tests and then we are ready to merge.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Just one more open comment. Thanks for adding the tests.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM, let's address the comment about the removal of the observer somewhere else.

@philipphofmann philipphofmann enabled auto-merge (squash) July 26, 2021 07:12
@philipphofmann philipphofmann merged commit b104211 into master Jul 26, 2021
@philipphofmann philipphofmann deleted the fix/dontcreatetransactionforrequest branch July 26, 2021 07:14
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.

Don't create a transaction for HTTP spans
4 participants