-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(tracing): Make BrowserTracing heartbeat interval configurable #5867
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
Conversation
f761887
to
07ed1fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this PR! On first glance this looks like a good change to me. We should try though to not make this breaking change (see my comment).
Would you mind adding a test that tests a custom heartbeat interval?
Side note:
When we merge this, we'll need to update the BrowserTracing options in docs as well. You obviously don't have to do this but I'm just putting it down here so we don't forget.
I've made |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the test! Looks good to me so far. One small doc change and then I think this is ready to 🚀
Thank you. |
@outsideris I just noticed that our linter check fails because of format errors in |
I fixed the lint errors. I missed it when I committed the suggestion on GitHub. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @outsideris! The PR looks good to me.
For the team: Let's wait with merging it until have a PR which updates our docs accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@outsideris apologies for missing this initially but the startIdleTransaction
signature needs to be changed because it's still potentially breaking. Would you mind making this change? If not we can do it too.
I fixed the breaking change and tests as well. |
Thanks @outsideris. Seems like GH Actions have an incident going on which is why CI won't run. Will take a final look tomorrow. Added a docs PR in the meantime: getsentry/sentry-docs#5606 |
Signed-off-by: Outsider <[email protected]>
Signed-off-by: Outsider <[email protected]>
Signed-off-by: Outsider <[email protected]>
Co-authored-by: Lukas Stracke <[email protected]>
Signed-off-by: Outsider <[email protected]>
Co-authored-by: Lukas Stracke <[email protected]>
Signed-off-by: Outsider <[email protected]>
I rebased this on latest master. |
I couldn't find why the tests failed. Any hints? |
Those were just flakes. I re-ran the failed jobs and they passed. |
Close #4925
The heartbeat interval was hard coded.
This PR makes heartbeat interval configurable via
BrowserTracingOptions
.