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

pkg(profiling-node): port profiling-node repo to monorepo #10151

Merged
merged 242 commits into from
Feb 2, 2024

Conversation

JonasBa
Copy link
Member

@JonasBa JonasBa commented Jan 11, 2024

Ports profiling-node to monorepo

This should only require CI step changes and no changes to the source code (except inheriting from base configs in monorepo, but those were already ported to profiling-node so they shouldn't result in any actual changes to the codebase).

TODO:

  • verify pkg cmds work and we have no lint/ts issues.
  • verify tests pass
  • verify ci commands work
  • prebuild binaries
  • port build to rollup
  • create e2e verdaccio config
  • ensure e2e tests pass and the app can build.

I'm opening this as ready to review. Currently e2e test on profiling fails as the package is missing. I'm not exactly sure why that is so hoping I can get a helping hand on that. The condition for a successful e2e test is to just execute a node script which triggers a profile and attempt to bundle profiling-node. The bundler test is there to ensure that we do in fact provide all of the statically required prebuild binaries and ensure bundlers can resolve them - else we risk breaking build tooling for folks bundling their code, which can be a common optimization in serverless environments.

The good:

  • Node profiling can resolve some issues around types which kept falling out fo sync with the SDK
  • We can follow the changes in JS SDK and core packages along as well as their versioning
  • The integration between the rest of the packages is tighter and safer, as it should be impossible for the sentry-javascript packages we rely on now to break in profiling-node.

The unfortunate:

  • CI timings will increase mostly because of the performance of windows runners. Installing repository dependencies on there takes roughly 10 minutes. I have tried leveraging the cache as much as I could, but since our dep paths are marked as ~/path/to/cached_dep and ${{github.workspace}}/path/to/dep it means they cannot be cached cross os as paths differ. I did not change those paths in this PR, but it can be an optimization as I suspect most of the packages are pure js and nothing would break.
  • When we pack artifacts in yarn build:tarball, we need skip profiling-node and assemble it separately. This is because we need to pull in the prebuilt binaries, else we'll only have the packed binary for the os we are running the action on. This same step needs to be replicated in e2e tests which prepare the tarball as well.

Couple of things I had to fix:

  • Our build tooling was not windows compatible (util polyfill was using awrong os path separator which wound up creating an incompatible build output on windows)
  • CI is finicky. I learned the hard way that node-gyp configure and build need to be sequential, else all hell breaks loose and for reasons which I didn't bother to investigate, python path and tooling is never correctly resolved (even when specified via gyp arg)

Sorry, something went wrong.

@AbhiPrasad AbhiPrasad self-requested a review January 11, 2024 16:00
@JonasBa JonasBa force-pushed the jb/pkg/profiling-node branch from 10caf39 to 14d19b4 Compare January 11, 2024 16:10
Copy link
Contributor

github-actions bot commented Jan 11, 2024

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped) 78.15 KB (0%)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 69.38 KB (0%)
@sentry/browser (incl. Tracing, Replay with Canvas) - Webpack (gzipped) 73.3 KB (0%)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped) 62.99 KB (0%)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 33.38 KB (0%)
@sentry/browser (incl. browserTracingIntegration) - Webpack (gzipped) 33.25 KB (0%)
@sentry/browser (incl. Feedback) - Webpack (gzipped) 31.33 KB (0%)
@sentry/browser (incl. sendFeedback) - Webpack (gzipped) 31.34 KB (0%)
@sentry/browser - Webpack (gzipped) 22.6 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped) 76.11 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 67.66 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 33.5 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped) 24.66 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 213.5 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 101.49 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 74.01 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 36.63 KB (0%)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 69.73 KB (0%)
@sentry/react - Webpack (gzipped) 22.63 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 86.5 KB (0%)
@sentry/nextjs Client - Webpack (gzipped) 50.8 KB (0%)
@sentry-internal/feedback - Webpack (gzipped) 17.21 KB (0%)

@JonasBa JonasBa force-pushed the jb/pkg/profiling-node branch from 8d98bdd to febd09b Compare January 11, 2024 21:40
@JonasBa
Copy link
Member Author

JonasBa commented Jan 17, 2024

I think I spent like 1 day on the fact that running lerna run build between node-gyp configure node-gyp build somehow breaks gyp...

@JonasBa
Copy link
Member Author

JonasBa commented Jan 17, 2024

Seems like we might be gucci on the prebuild, however fetching dep for the repo is so slow on windows it times out. Going to try and bump it

@JonasBa JonasBa requested review from anonrig, lforst, Lms24 and mydea January 19, 2024 15:19
"build:transpile": {
"dependsOn": [
"^build:transpile",
"^build:types"
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be included in the nx.json in root?

@JonasBa JonasBa marked this pull request as ready for review January 20, 2024 17:12
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

CI timings will increase mostly because of the performance of windows runners

We just need bigger runners, but lets see how much this slows us down first after we merge.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

All good on my side! Let's not merge just yet - would like a team member to sanity check craft and CI config

- name: Extract Profiling Node Prebuilt Binaries
# @TODO: v4 breaks convenient merging of same name artifacts
# https://github.com/actions/upload-artifact/issues/478
uses: actions/download-artifact@v3
Copy link
Member

Choose a reason for hiding this comment

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

Can we use the workaround in actions/upload-artifact#478 (comment) here?

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.

Generally looks good to me! Still had a couple of questions regarding publishing config.

As well as two requests:

  • If not done yet, can you go through docs/new-sdk-release-checklist.md to make sure we're not missing something? Some points might not apply, given it's a migration rather than a first time publish but it probably makes sense to check anyway.
  • Also, I'd recommend pushing a release/test branch to the repo and checking the build artifacts to make sure that everything is in the tarball as expected. From what I've seen we should be good but it probably makes sense anyway to double check.

Last question: Once we cut the first release with this package, the version will jump from 1.x to 7.x. Are you fine with that or do we need to stay on 1.x for a while? (maybe that's the reason for the separate version bump files but I'm not sure if craft/our publishing can actually handle this).

@JonasBa
Copy link
Member Author

JonasBa commented Jan 23, 2024

Generally looks good to me! Still had a couple of questions regarding publishing config.

As well as two requests:

  • If not done yet, can you go through docs/new-sdk-release-checklist.md to make sure we're not missing something? Some points might not apply, given it's a migration rather than a first time publish but it probably makes sense to check anyway.

Yes, definitely. I did not know it existed, but I'll go through :)

  • Also, I'd recommend pushing a release/test branch to the repo and checking the build artifacts to make sure that everything is in the tarball as expected. From what I've seen we should be good but it probably makes sense anyway to double check.

Good idea.

Last question: Once we cut the first release with this package, the version will jump from 1.x to 7.x. Are you fine with that or do we need to stay on 1.x for a while? (maybe that's the reason for the separate version bump files but I'm not sure if craft/our publishing can actually handle this).

Yes that is ok. I also removed node 14 from the precompile matrix so a new major change will be welcome

@JonasBa
Copy link
Member Author

JonasBa commented Jan 23, 2024

@lms @AbhiPrasad to minimize impact on CI times, I made the change to only run the compile bindings job if profiling or nodejs package has changed or if we are on a release branch.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Let's merge!

The amount of beers I owe you continue to climb @JonasBa - I'll be bankrupt at this rate 🤣

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.

Yup let's go! Thanks for applying my suggestions!

@JonasBa
Copy link
Member Author

JonasBa commented Jan 25, 2024

@lms @AbhiPrasad let me update this branch and merge it first thing start of next week. I will also update final artifact action to v4

@JonasBa JonasBa force-pushed the jb/pkg/profiling-node branch from 0a9387a to 8fa4055 Compare January 29, 2024 19:33

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@JonasBa JonasBa force-pushed the jb/pkg/profiling-node branch from e10e477 to 0ba4761 Compare February 1, 2024 19:29
@JonasBa JonasBa merged commit e6597b9 into develop Feb 2, 2024
@JonasBa JonasBa deleted the jb/pkg/profiling-node branch February 2, 2024 02:15
onurtemizkan pushed a commit that referenced this pull request Feb 4, 2024
Ports profiling-node to monorepo

This should only require CI step changes and no changes to the source
code (except inheriting from base configs in monorepo, but those were
already ported to profiling-node so they shouldn't result in any actual
changes to the codebase).

TODO:
 - [x] verify pkg cmds work and we have no lint/ts issues.
 - [x] verify tests pass
 - [x] verify ci commands work
 - [x] prebuild binaries
 - [x] port build to rollup
 - [x] create e2e verdaccio config
 - [x] ensure e2e tests pass and the app can build.
 
 
I'm opening this as ready to review. Currently e2e test on profiling
fails as the package is missing. I'm not exactly sure why that is so
hoping I can get a helping hand on that. The condition for a successful
e2e test is to just execute a node script which triggers a profile and
attempt to bundle profiling-node. The bundler test is there to ensure
that we do in fact provide all of the statically required prebuild
binaries and ensure bundlers can resolve them - else we risk breaking
build tooling for folks bundling their code, which can be a common
optimization in serverless environments.
 
 The good:
- Node profiling can resolve some issues around types which kept falling
out fo sync with the SDK
- We can follow the changes in JS SDK and core packages along as well as
their versioning
- The integration between the rest of the packages is tighter and safer,
as it should be impossible for the sentry-javascript packages we rely on
now to break in profiling-node.
  
The unfortunate:
- CI timings will increase mostly because of the performance of windows
runners. Installing repository dependencies on there takes roughly 10
minutes. I have tried leveraging the cache as much as I could, but since
our dep paths are marked as `~/path/to/cached_dep` and
`${{github.workspace}}/path/to/dep` it means they cannot be cached cross
os as paths differ. I did not change those paths in this PR, but it can
be an optimization as I suspect most of the packages are pure js and
nothing would break.
- When we pack artifacts in yarn build:tarball, we need skip
profiling-node and assemble it separately. This is because we need to
pull in the prebuilt binaries, else we'll only have the packed binary
for the os we are running the action on. This same step needs to be
replicated in e2e tests which prepare the tarball as well.

Couple of things I had to fix:
- Our build tooling was not windows compatible (util polyfill was using
awrong os path separator which wound up creating an incompatible build
output on windows)
- CI is finicky. I learned the hard way that node-gyp configure and
build need to be sequential, else all hell breaks loose and for reasons
which I didn't bother to investigate, python path and tooling is never
correctly resolved (even when specified via gyp arg)
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.

None yet

4 participants