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

feat(tracing): Make BrowserTracing heartbeat interval configurable #5867

Merged
merged 7 commits into from Oct 7, 2022

Conversation

outsideris
Copy link
Contributor

@outsideris outsideris commented Oct 2, 2022

Close #4925

The heartbeat interval was hard coded.
This PR makes heartbeat interval configurable via BrowserTracingOptions.

@outsideris outsideris force-pushed the issue-4925 branch 2 times, most recently from f761887 to 07ed1fc Compare October 3, 2022 11:17
Copy link
Member

@Lms24 Lms24 left a 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.

packages/tracing/src/hubextensions.ts Outdated Show resolved Hide resolved
@Lms24 Lms24 changed the title feat(browser): make heartbeat interval configurable feat(tracing): Make BrowserTracing heartbeat interval configurable Oct 3, 2022
@outsideris
Copy link
Contributor Author

I've made heartbeatInterval optional and add a test.
Please check new test is correct.

Copy link
Member

@Lms24 Lms24 left a 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 🚀

packages/tracing/src/browser/browsertracing.ts Outdated Show resolved Hide resolved
@outsideris
Copy link
Contributor Author

Thank you.

@Lms24
Copy link
Member

Lms24 commented Oct 4, 2022

@outsideris I just noticed that our linter check fails because of format errors in browsertracing.ts. Would you mind running yarn fix in the browser package? yarn lint should give you more insights into what's causing the problem.

@outsideris
Copy link
Contributor Author

I fixed the lint errors. I missed it when I committed the suggestion on GitHub.

Lms24
Lms24 previously approved these changes Oct 4, 2022
Copy link
Member

@Lms24 Lms24 left a 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.

Copy link
Member

@Lms24 Lms24 left a 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.

packages/tracing/src/hubextensions.ts Outdated Show resolved Hide resolved
@Lms24 Lms24 dismissed their stale review October 5, 2022 08:14

Missed breaking change

@outsideris
Copy link
Contributor Author

I fixed the breaking change and tests as well.

@Lms24
Copy link
Member

Lms24 commented Oct 5, 2022

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

outsideris and others added 7 commits October 6, 2022 02:08
Signed-off-by: Outsider <outsideris@gmail.com>
Signed-off-by: Outsider <outsideris@gmail.com>
Signed-off-by: Outsider <outsideris@gmail.com>
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
Signed-off-by: Outsider <outsideris@gmail.com>
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
Signed-off-by: Outsider <outsideris@gmail.com>
@outsideris
Copy link
Contributor Author

I rebased this on latest master.

@outsideris
Copy link
Contributor Author

I couldn't find why the tests failed. Any hints?

@Lms24
Copy link
Member

Lms24 commented Oct 6, 2022

I couldn't find why the tests failed. Any hints?

Those were just flakes. I re-ran the failed jobs and they passed.

@Lms24 Lms24 merged commit 1d5c610 into getsentry:master Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make heartbeat interval configurable
3 participants