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 traceOptionsRequests option to disable tracing of OPTIONS requests #2453

Merged
merged 4 commits into from
Jan 9, 2023

Conversation

adinauer
Copy link
Member

@adinauer adinauer commented Jan 5, 2023

📜 Description

traceOptionsRequests defaults to true for now. I think we should change this to false only in the next major release.

💡 Motivation and Context

Allows us to fix #2231 in the next major by flipping the default to false.

💚 How did you test it?

Unit Tests, manually

📝 Checklist

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

🔮 Next steps

@@ -387,6 +387,10 @@ public class SentryOptions {

private @NotNull IMemoryCollector memoryCollector = NoOpMemoryCollector.getInstance();

// TODO this should default to false on the next major
Copy link
Member Author

Choose a reason for hiding this comment

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

Do you agree with this approach of having it default to true until the next major then change it to the desired default of false?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good 👍

@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against e5d6ce6

@adinauer
Copy link
Member Author

adinauer commented Jan 5, 2023

Docs PR here: getsentry/sentry-docs#6019

@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 340.10 ms 377.98 ms 37.88 ms
Size 1.73 MiB 2.33 MiB 616.92 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
4a9c176 320.62 ms 334.68 ms 14.06 ms
a04f788 321.78 ms 354.12 ms 32.35 ms
f1150bc 306.15 ms 306.58 ms 0.44 ms
d00c464 337.43 ms 387.57 ms 50.15 ms
f55020d 338.00 ms 370.74 ms 32.74 ms
4a9c176 319.77 ms 363.20 ms 43.43 ms
703d523 275.51 ms 323.02 ms 47.51 ms
ecf9680 321.55 ms 385.52 ms 63.97 ms
b85d8aa 289.35 ms 335.92 ms 46.56 ms
90e9745 314.68 ms 357.28 ms 42.60 ms

App size

Revision Plain With Sentry Diff
4a9c176 1.73 MiB 2.33 MiB 612.69 KiB
a04f788 1.73 MiB 2.32 MiB 609.88 KiB
f1150bc 1.73 MiB 2.33 MiB 615.66 KiB
d00c464 1.73 MiB 2.33 MiB 613.02 KiB
f55020d 1.73 MiB 2.33 MiB 616.54 KiB
4a9c176 1.73 MiB 2.33 MiB 612.69 KiB
703d523 1.73 MiB 2.33 MiB 613.23 KiB
ecf9680 1.73 MiB 2.32 MiB 612.39 KiB
b85d8aa 1.73 MiB 2.32 MiB 611.62 KiB
90e9745 1.73 MiB 2.32 MiB 608.63 KiB

Previous results on branch: feat/trace-options-requests

Startup times

Revision Plain With Sentry Diff
e11f8c4 340.32 ms 391.90 ms 51.58 ms

App size

Revision Plain With Sentry Diff
e11f8c4 1.73 MiB 2.33 MiB 616.92 KiB

@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2023

Codecov Report

Base: 80.15% // Head: 80.14% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (c288e9c) compared to base (8ade225).
Patch coverage: 66.66% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2453      +/-   ##
============================================
- Coverage     80.15%   80.14%   -0.02%     
- Complexity     3828     3836       +8     
============================================
  Files           310      310              
  Lines         14466    14473       +7     
  Branches       1914     1915       +1     
============================================
+ Hits          11595    11599       +4     
- Misses         2121     2124       +3     
  Partials        750      750              
Impacted Files Coverage Δ
sentry/src/main/java/io/sentry/SentryOptions.java 79.54% <25.00%> (-0.51%) ⬇️
...ry/spring/jakarta/tracing/SentryTracingFilter.java 86.27% <100.00%> (+0.27%) ⬆️
.../io/sentry/spring/tracing/SentryTracingFilter.java 86.53% <100.00%> (+0.53%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -387,6 +387,10 @@ public class SentryOptions {

private @NotNull IMemoryCollector memoryCollector = NoOpMemoryCollector.getInstance();

// TODO this should default to false on the next major
Copy link
Member

Choose a reason for hiding this comment

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

Sounds good 👍

@adinauer adinauer merged commit 94ebe46 into main Jan 9, 2023
@adinauer adinauer deleted the feat/trace-options-requests branch January 9, 2023 09:37
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.

Do not start transaction for OPTIONS requests
4 participants