sentry-javascript icon indicating copy to clipboard operation
sentry-javascript copied to clipboard

feat(node): Populate `event.contexts` for Node.js

Open timfish opened this issue 3 years ago • 4 comments
trafficstars

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

timfish avatar Aug 02 '22 13:08 timfish

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 avatar Aug 02 '22 14:08 timfish

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 avatar Aug 02 '22 15:08 timfish

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.

timfish avatar Aug 02 '22 16:08 timfish

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

AbhiPrasad avatar Aug 05 '22 18:08 AbhiPrasad

Hopefully https://github.com/getsentry/sentry-javascript/pull/5579 will unblock the test failures for this PR.

AbhiPrasad avatar Aug 15 '22 14:08 AbhiPrasad

With https://github.com/getsentry/sentry-javascript/pull/5579 getting merged in, we can rebase this and see if that helps with the test failures!

AbhiPrasad avatar Aug 16 '22 13:08 AbhiPrasad

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

AbhiPrasad avatar Aug 16 '22 16:08 AbhiPrasad

I'm still getting what looks like leakage between tests. When I run them locally they all pass on first run and then failures on the next run. Test failures vary each run. For example, this looks like the express test leaks into the pg test:

image

timfish avatar Aug 17 '22 07:08 timfish

@timfish, I'm working on it. 👍

onurtemizkan avatar Aug 17 '22 10:08 onurtemizkan

Alright https://github.com/getsentry/sentry-javascript/pull/5596 has been merged in! We can try rebasing and trying again here!

AbhiPrasad avatar Aug 18 '22 14:08 AbhiPrasad

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

timfish avatar Aug 21 '22 17:08 timfish

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?

timfish avatar Aug 22 '22 15:08 timfish

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.

onurtemizkan avatar Aug 22 '22 16:08 onurtemizkan

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

AbhiPrasad avatar Aug 29 '22 15:08 AbhiPrasad

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

timfish avatar Aug 29 '22 18:08 timfish

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

AbhiPrasad avatar Aug 29 '22 19:08 AbhiPrasad

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

Venipa avatar Sep 29 '22 12:09 Venipa

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.

AbhiPrasad avatar Sep 29 '22 12:09 AbhiPrasad