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

[Replay] Migrating sentry-replay into JS SDK mono-repo #5326

Closed
8 tasks done
smeubank opened this issue Jun 28, 2022 · 10 comments
Closed
8 tasks done

[Replay] Migrating sentry-replay into JS SDK mono-repo #5326

smeubank opened this issue Jun 28, 2022 · 10 comments
Assignees
Labels
Package: replay Issues related to the Sentry Replay SDK Type: Improvement

Comments

@smeubank
Copy link
Member

smeubank commented Jun 28, 2022

Problem Statement

There is the sentry-replay repo which has the Sentry Browser SDK package and rreweb packages as dependencies.

Meaning it has clear requirement dependancies, and similar requirement for CI/CD in terms of testing and creating releases, but cannot easily take advantage of everything which is already part of the JS SDK.

Possible benefits of moving into the JS mono-repo

  • existing test infrastructure
  • monitor file size over across changes
  • existing releases
  • could simplify reuse of code from within the JS SDK and for users to instrument in the future

Concerns:
Adds more dependencies into the JS repo, where there is already quite a number of integrations for different browser and Node frameworks, along with all the Sentry features, and SDKs which are dependant on the JS SDK (Electron and react-native)

Solution Brainstorm

Include sentry-replay to be more tightly coupled as part of the Sentry JavaScript SDK mono-repo

Outcomes:

Progress

Phase I: Pre-Migration

Phase II: Migration

Phase III: Post-Migration

More long-term tasks are tracked in

@AbhiPrasad
Copy link
Member

Considering users already get very confused over conflicting versions between @sentry/electron and @sentry/react-native and the core deps (@sentry/core or @sentry/utils), I think keeping Replay in sync with the rest of the main dependencies is very beneficial for product experience.

We'll have to take some care to make sure that the test/build infra does not suffer as a result though - that IMO will fully make it clear if we can handle the extra dependencies.

Re-using the playwright test suite might be useful for the replay sdk so they can validate with more robust integration tests.

could simplify reuse of code from within the JS SDK and for users to instrument in the future

This should be a priority so that we make sure that bundle size is saved as much as possible. For example, the replay SDK instruments network requests like our performance monitoring SDK does, and duplicating that code is unnecessary.

@AbhiPrasad
Copy link
Member

There's another set of organizational challenges that will have to be handled here. How do we handle the backlog? Do we use labels or a different project to triage/categorize incoming Replay related GH issues. How do we route things appropriately?

@github-actions
Copy link
Contributor

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@mitsuhiko
Copy link
Member

Not ded according to slack :)

@smeubank
Copy link
Member Author

@billyvg and @jas-kas we can already start collaborating here if we need to add to this issue, or split it up

@AbhiPrasad
Copy link
Member

One important decision to be made here. Does @sentry/replay follow the same versioning structure as the rest of the repo? If it doesn't we need to adjust our publishing process around that!

@vladanpaunovic
Copy link
Contributor

One important decision to be made here. Does @sentry/replay follow the same versioning structure as the rest of the repo? If it doesn't we need to adjust our publishing process around that!

I discussed this with @benvinegar and we agreed that @sentry/replay will follow the same versioning structure as the rest of the monorepo.

We will make sure to highlight in docs and release notes that this is still in alpha before jumping into a stable version.

@jas-kas
Copy link
Member

jas-kas commented Oct 28, 2022

Questions about the potential benefits of the migration:

  1. Tree-shaking architecture existing the JS SDK mono-repo --> This requires testing to see if it'll work OOTB. Is this a hard requirement?

  2. Bundle size improvements --> Can we cut out some compression code to make this possible? Is this work we should scope in?

cc @benvinegar @billyvg @smeubank

@billyvg
Copy link
Member

billyvg commented Oct 31, 2022

Questions about the potential benefits of the migration:

  1. Tree-shaking architecture existing the JS SDK mono-repo --> This requires testing to see if it'll work OOTB. Is this a hard requirement?

Not sure I follow here, but don't think it is a hard requirement.

  1. Bundle size improvements --> Can we cut out some compression code to make this possible? Is this work we should scope in?

We can explore different libraries, but we will want to keep the compression feature. Lazy loading the compression library is a possibility.

@Lms24 Lms24 self-assigned this Dec 5, 2022
@Lms24 Lms24 changed the title Migrating sentry-replay into JS SDK mono-repo [Replay] Migrating sentry-replay into JS SDK mono-repo Dec 5, 2022
@Lms24 Lms24 added the Package: replay Issues related to the Sentry Replay SDK label Dec 5, 2022
@Lms24
Copy link
Member

Lms24 commented Dec 7, 2022

Closing this issue as the initial migration is completed. I'll extract the left-over tasks into two new issues to enable better prioritization

Edit: see #6458 and #6459

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: replay Issues related to the Sentry Replay SDK Type: Improvement
Projects
None yet
Development

No branches or pull requests

8 participants