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(node): Populate event.contexts for Node.js #5512

Merged
merged 13 commits into from Aug 29, 2022

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Aug 2, 2022

Much of this code has been in use in the Electron SDK for a long time:
https://github.com/getsentry/sentry-electron/blob/master/src/main/context.ts
The addition of memory and cpu info was more recent:
https://github.com/getsentry/sentry-electron/blob/master/src/main/integrations/additional-context.ts

Closes #5500

@AbhiPrasad AbhiPrasad requested review from a team, lobsterkatie and AbhiPrasad and removed request for a team August 2, 2022 13:57
AbhiPrasad
AbhiPrasad previously approved these changes Aug 2, 2022
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.

Can we add unit/integration tests for this? We can do it in a follow up PR.

packages/types/src/context.ts Show resolved Hide resolved
@timfish
Copy link
Collaborator Author

timfish commented Aug 2, 2022

I think the Node integration tests are failing because this change introduces an async event processor and this in turn causes the session/event envelopes to be received in a different order.

@timfish
Copy link
Collaborator Author

timfish commented Aug 2, 2022

The async OS evaluation needs to handle reentrancy properly.

If a second events occurs before the async task has completed, _cachedContexts will be defined but not fully populated. _cachedContexts should therefore be a Promise which is awaited every time.

@timfish
Copy link
Collaborator Author

timfish commented Aug 2, 2022

Now only the dynamic properties are calculated on every event and everything else is cached.

We don't call _getContexts() from the constructor because on some platforms this will cause it to read files and spawn processes immediately, even if an event is never sent.

@AbhiPrasad AbhiPrasad dismissed their stale review August 2, 2022 17:32

Need to look at async issues

@AbhiPrasad
Copy link
Member

Hey Tim, any updates on this? Any help you need from our side to investigate or look into stuff?

@AbhiPrasad
Copy link
Member

Hopefully #5579 will unblock the test failures for this PR.

@AbhiPrasad
Copy link
Member

With #5579 getting merged in, we can rebase this and see if that helps with the test failures!

@AbhiPrasad
Copy link
Member

Do we have to revert some of the previous test fixes?

@timfish

This comment was marked as outdated.

@onurtemizkan
Copy link
Collaborator

@timfish, I'm working on it. 👍

@AbhiPrasad
Copy link
Member

Alright #5596 has been merged in! We can try rebasing and trying again here!

@timfish
Copy link
Collaborator Author

timfish commented Aug 21, 2022

I've been away for a few days but will get back onto this on Monday!

@timfish
Copy link
Collaborator Author

timfish commented Aug 22, 2022

Tests are all passing 🎉🎉🎉🎉🎉

Thanks @onurtemizkan for all your hard work fixing this for me!

I see there are some new methods for fetching single events. Is it a better idea to use those over my new filtering method?

@onurtemizkan
Copy link
Collaborator

I see there are some new methods for fetching single events. Is it a better idea to use those over my new filtering method?

Moving filters inside the nock interceptors helped keep the required event / transaction count by type, so we don't need to jump indices anymore. There were old flaky test cases that were jumping indices by 3 and that was hard to write / debug.

@AbhiPrasad
Copy link
Member

Alright, so I think once we swap filterEnvelopeItems for the new methods, we should be good to merge this!

@timfish
Copy link
Collaborator Author

timfish commented Aug 29, 2022

All done! I also replaced getMultipleEnvelopeRequest with getEnvelopeRequest where appropriate.

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.

🚀

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.

Based on changes in getsentry/develop#658

packages/types/src/context.ts Outdated Show resolved Hide resolved
packages/node/src/integrations/context.ts Outdated Show resolved Hide resolved
packages/node/src/integrations/context.ts Outdated Show resolved Hide resolved
@AbhiPrasad AbhiPrasad enabled auto-merge (squash) August 29, 2022 19:03
@AbhiPrasad AbhiPrasad merged commit e8c78c4 into getsentry:master Aug 29, 2022
@AbhiPrasad
Copy link
Member

Thanks so much for your patience @timfish - and thanks @onurtemizkan for the fixes!!!

@Venipa
Copy link

Venipa commented Sep 29, 2022

has this been tested on serverless environments? like azure functions?

stacktrace:

RangeError: Invalid time value
  ?, in Date.toISOString
  File "C:\home\site\wwwroot\node_modules\@sentry\node\cjs\integrations\context.js", line 177, col 64, in getDeviceContext
  File "C:\home\site\wwwroot\node_modules\@sentry\node\cjs\integrations\context.js", line 95, col 25, in Context._getContexts
  File "C:\home\site\wwwroot\node_modules\@sentry\node\cjs\integrations\context.js", line 51, col 46, in Context.addContext

@AbhiPrasad
Copy link
Member

Hey @Venipa could you open a new issue re: your problem? Would be great to get a) your node version, b) the JS runtime that azure functions uses

It would be that process.uptime is causing problems.

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.

Include more information in event.contexts for Node.js
4 participants