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

fix(node): Only set contexts runtime if not set. #5226

Closed
wants to merge 1 commit into from

Conversation

AbhiPrasad
Copy link
Member

Make sure we don't overwrite contexts runtime if it has already been set.

Fixes issue addressed in #5190 (comment).

cc @timfish

Make sure we don't overwrite contexts runtime if it has already been
set.
@timfish
Copy link
Collaborator

timfish commented Jun 7, 2022

For a minute I thought I could just work around this by overwriting again in the Electron SDK normalisation code. But nope, it still gets sent with native crashes.

I'm guessing this might also be an issue with server_name since we delete that too and wont be able to exclude it if its added here 🤔

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.43 KB (added)
@sentry/browser - ES5 CDN Bundle (minified) 60.2 KB (added)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.23 KB (added)
@sentry/browser - ES6 CDN Bundle (minified) 53.82 KB (added)
@sentry/browser - Webpack (gzipped + minified) 19.99 KB (added)
@sentry/browser - Webpack (minified) 65.19 KB (added)
@sentry/react - Webpack (gzipped + minified) 20.02 KB (added)
@sentry/nextjs Client - Webpack (gzipped + minified) 43.95 KB (added)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 25.56 KB (added)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.11 KB (added)

@timfish
Copy link
Collaborator

timfish commented Jun 7, 2022

Come to think of it, wasn't server_name in tags before?

@AbhiPrasad
Copy link
Member Author

For a minute I thought I could just work around this by overwriting again in the Electron SDK normalisation code. But nope, it still gets sent with native crashes.

I'm guessing this might also be an issue with server_name since we delete that too and wont be able to exclude it if its added here 🤔

This should help with that right?

Come to think of it, wasn't server_name in tags before?

Nope, it was always sent in event. https:/getsentry/sentry-javascript/blame/d48f6fd79d4d860264868b6ae653cd902c87cd14/packages/node/src/client.ts#L143

@timfish
Copy link
Collaborator

timfish commented Jun 7, 2022

It was also set in tags here:
https:/getsentry/sentry-javascript/pull/5190/files#diff-010050dae6ec8db250b7ec59fbcb1be31cdb2f0d0b267b47b0abe61265de4855L61

It looks like it was only set in event when the option was supplied:
https:/getsentry/sentry-javascript/pull/5190/files#diff-0bc2a7799abc5214e271ffd7a87576199c3ac172c70e73ee89bdaccd3b0a330fL142-L144

@AbhiPrasad
Copy link
Member Author

It was also set in tags here

This was just in serverless - not for node in general.

I think though for server_name, there's no good way around it. We probably need to just stuff this all in an integration then.

@timfish
Copy link
Collaborator

timfish commented Jun 7, 2022

I've managed to override contexts.runtime for all events in the Electron SDK now. The issue was that for native crashes I was calling client.captureEvent vs captureEvent from core so it wasn't hitting the normalisation.

This was just in serverless - not for node in general.

Ah my bad!

I was just looking at this really ancient code from the Electron SDK which probably hasn't been doing what it should for a long while:
https:/getsentry/sentry-electron/blob/d5ab6fa9d693ffd1207250b1d6069c72f9e4d726/src/common/normalize.ts#L64-L68

@AbhiPrasad
Copy link
Member Author

Closing as it should be taken care of by #5257

@AbhiPrasad AbhiPrasad closed this Jun 15, 2022
@AbhiPrasad AbhiPrasad deleted the abhi-node-runtime branch June 15, 2022 14:25
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.

2 participants