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

Jump to bottom

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 7 commits into from
+75 −23

Conversation

outsideris
Copy link
Contributor

@outsideris outsideris commented Oct 2, 2022 •
edited by Lms24

⚠️ Only merge once docs update PR is ready! (edited by @Lms24)

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 Oct 3, 2022
Lms24
Lms24 reviewed Oct 3, 2022
View changes
Copy link
Member

@Lms24 Lms24 left a comment

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
@@ -186,13 +186,14 @@ export function startIdleTransaction(
transactionContext: TransactionContext,
idleTimeout: number,
finalTimeout: number,
heartbeatInterval: number,
Copy link
Member

@Lms24 Lms24 Oct 3, 2022

Choose a reason for hiding this comment

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

Can we make heartbeatInterval an optional parameter? startIdleTransaction is publicly exposed and if heartbeatInterval is a mandatory param, this would be a breaking change.

Copy link
Contributor Author

@outsideris outsideris Oct 3, 2022

Choose a reason for hiding this comment

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

Agreed!

@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

outsideris commented Oct 3, 2022

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

Lms24
Lms24 reviewed Oct 3, 2022
View changes
Copy link
Member

@Lms24 Lms24 left a comment

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

outsideris commented Oct 3, 2022

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

outsideris commented Oct 4, 2022

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

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

@Lms24 Lms24 left a comment •
edited

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.

Lms24
Lms24 reviewed Oct 5, 2022
View changes
Copy link
Member

@Lms24 Lms24 left a comment

@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 Oct 5, 2022

Missed breaking change

@outsideris
Copy link
Contributor Author

outsideris commented Oct 5, 2022

I fixed the breaking change and tests as well.

@Lms24 Lms24 mentioned this pull request Oct 5, 2022
Open
@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 Oct 5, 2022
@outsideris
feat(browser): make heartbeat interval configurable
0d1b6cc 
Signed-off-by: Outsider <outsideris@gmail.com>
@outsideris
feat(browser): make heartbeatInterval optional
a7b0cfc 
Signed-off-by: Outsider <outsideris@gmail.com>
@outsideris
feat(browser): test for heartbeatInterval
7294db1 
Signed-off-by: Outsider <outsideris@gmail.com>
@outsideris @Lms24
Update packages/tracing/src/browser/browsertracing.ts
60ab075 
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
@outsideris
feat(browser): fix lint errors
aa1b794 
Signed-off-by: Outsider <outsideris@gmail.com>
@outsideris @Lms24
Update packages/tracing/src/hubextensions.ts
8f7ca31 
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
@outsideris
fix tests as startIdleTransaction signature
fafb533 
Signed-off-by: Outsider <outsideris@gmail.com>
@outsideris
Copy link
Contributor Author

outsideris commented Oct 5, 2022

I rebased this on latest master.

@outsideris
Copy link
Contributor Author

outsideris commented Oct 5, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lms24 Lms24

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make heartbeat interval configurable
2 participants
@outsideris @Lms24