feat(processing): Enable Node.js Source Maps #6626

kamilogorek · 2017-11-27T11:03:02Z

I still need to add proper tests for this but wanted to make sure that I'm on the right track and my implementation is decent enough.

mitsuhiko · 2017-11-27T11:50:31Z

src/sentry/lang/javascript/processor.py

+        # therefore we only process user-land frames (starting with /)
+        # or those created by bundle/webpack internals
+        if self.data.get('platform') == 'node' and \
+            not frame.get('abs_path').startswith(('/', 'app:', 'webpack:')):


Are we sure this is correct? There is no rule about which files might or might not appear in sourcemaps. For react-native i'm pretty sure we saw a bare 'index.bundle' as filename.

I cannot give a definite answer, but I wasn't able to attain any other result than the one above and I tried a lot of variants.

mitsuhiko · 2017-11-27T11:51:37Z

src/sentry/models/featureadoption.py

@@ -50,7 +50,7 @@
 )
 manager.add(43, "user_tracking", "User Tracking", "code", prerequisite=["first_event"])
 manager.add(44, "custom_tags", "Custom Tags", "code", prerequisite=["first_event"])
-manager.add(45, "source_maps", "Source Maps", "code", prerequisite=["first_event", "javascript"])
+manager.add(45, "source_maps", "Source Maps", "code", prerequisite=["first_event", ("javascript", "node")])


Does this work?

Yes, see

sentry/src/sentry/models/featureadoption.py

Line 62 in f4a19b6

prerequisite=["first_event", ("python", "javascript", "node", "php")]

Just some feedback here, but this is ultimately @ehfeng's decision:

I'm not sure I'd group node under this feature tracking flag thing. Source maps in node are generally less common and I wouldn't say "required" in the same sense as they are for browser js. And not sure if we'd want to say, using node sourcemaps also toggles this for browser js since I assume setup is going to be entirely different.

mattrobenolt

Overall, this is fine, but I'm really curious how this is going to look on the other end and how raven-node is adapting to send the correct data we need.

Ideally, we're determining if we need sourcemaps from the SDK first, and making sure we tag the payloads correctly so it can be identified. The difference here, which is probably obvious, is that sometimes Node doesn't need it, and sometimes it does. Whereas with JS today, we make strong assumptions that we 100% need additional processing.

Mostly just iterating that the discovery side of things I think needs to happen within the SDK since it'd know easier than the server. Unless you have a better idea for how the whole picture works, I might be thinking of things differently.

mattrobenolt · 2017-11-27T13:28:53Z

src/sentry/models/featureadoption.py

@@ -50,7 +50,7 @@
 )
 manager.add(43, "user_tracking", "User Tracking", "code", prerequisite=["first_event"])
 manager.add(44, "custom_tags", "Custom Tags", "code", prerequisite=["first_event"])
-manager.add(45, "source_maps", "Source Maps", "code", prerequisite=["first_event", "javascript"])
+manager.add(45, "source_maps", "Source Maps", "code", prerequisite=["first_event", ("javascript", "node")])


Just some feedback here, but this is ultimately @ehfeng's decision:

I'm not sure I'd group node under this feature tracking flag thing. Source maps in node are generally less common and I wouldn't say "required" in the same sense as they are for browser js. And not sure if we'd want to say, using node sourcemaps also toggles this for browser js since I assume setup is going to be entirely different.

mitsuhiko · 2017-11-27T13:42:15Z

@mattrobenolt the way I would like to treat this going forward is that SDK does not determine processing. For some cases we will want to move processing into the relay agent at one point and the less specific config is required to trigger that the better.

kamilogorek · 2017-11-27T14:32:19Z

The main difference between browser JS and node JS is that latter requires both, frame URLs rewrites and manual source maps upload (or using our webpack plugin, as I do now locally – will add it as a doc entry). Where for the browser it can be skipped completely, as we can pull the code and maps from the server (and/or scrape the modules if necessary).

kamilogorek · 2017-11-28T13:01:56Z

I'm not sure if we shouldn't disable scraping for node:

if self.data.get('platform') == 'node':
  self.allow_scraping = False
else:
  self.allow_scraping = self.project.get_option('sentry:scrape_javascript', True)

kamilogorek · 2017-11-28T14:19:53Z

@mitsuhiko I tried to use both tactics, mocking fetch_file and fixtures, but I'm stuck on parsing source maps (without it, I believe it won't move to the branch we need, as token is always None now). It's way over my Python "skills"... ¯\(ツ)/¯

ghost · 2017-11-28T14:20:10Z

	1 Warning
⚠️	You should update CHANGES due to the size of this PR

Generated by 🚫 danger

mitsuhiko · 2017-11-28T15:51:25Z

@kamilogorek if you want i can finish the tests

kamilogorek · 2017-11-28T17:51:25Z

That'd be very helpful, thanks @mitsuhiko! I need some assistance delivering this feature.

Tests are the only thing left now.

dcramer · 2017-12-21T22:59:54Z

merging and praying per @kamilogorek's request

kamilogorek requested a review from mitsuhiko November 27, 2017 11:03

mitsuhiko suggested changes Nov 27, 2017

View reviewed changes

kamilogorek mentioned this pull request Nov 27, 2017

fix: access 'process' through global variable getsentry/raven-node#399

Merged

mattrobenolt reviewed Nov 27, 2017

View reviewed changes

kamilogorek mentioned this pull request Nov 29, 2017

docs: Include Source Maps docs getsentry/raven-node#400

Merged

kamilogorek assigned mitsuhiko Nov 30, 2017

mitsuhiko approved these changes Dec 20, 2017

View reviewed changes

kamilogorek and others added 3 commits December 21, 2017 23:00

feat(processing): Enable Node.js Source Maps

b84f861

test(javascript): Properly assert that we are not fetching in JS

d1f184c

test(sourcemaps): Fixed tests for node sourcemaps

d4f7e96

kamilogorek force-pushed the nodejs-sourcemaps branch from 85a4f08 to d4f7e96 Compare December 21, 2017 22:00

dcramer merged commit c5d2742 into master Dec 21, 2017

dcramer deleted the nodejs-sourcemaps branch December 21, 2017 22:59

netproteus mentioned this pull request Jan 3, 2018

Support sourcemaps for Node #2632

Closed

marcelgoya mentioned this pull request May 27, 2018

Sourcemaps were uploaded but Sentry displays only generated JavaScript code getsentry/raven-node#435

Open

github-actions bot locked and limited conversation to collaborators Dec 22, 2020

Uh oh!

feat(processing): Enable Node.js Source Maps #6626

feat(processing): Enable Node.js Source Maps #6626

Uh oh!

Conversation

kamilogorek commented Nov 27, 2017

Uh oh!

mitsuhiko Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

kamilogorek Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

mitsuhiko Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

kamilogorek Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

mattrobenolt Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

mattrobenolt left a comment

Choose a reason for hiding this comment

Uh oh!

mattrobenolt Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

mitsuhiko commented Nov 27, 2017

Uh oh!

kamilogorek commented Nov 27, 2017

Uh oh!

kamilogorek commented Nov 28, 2017

Uh oh!

kamilogorek commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Nov 28, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mitsuhiko commented Nov 28, 2017

Uh oh!

kamilogorek commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcramer commented Dec 21, 2017

Uh oh!

Uh oh!

kamilogorek commented Nov 28, 2017 •

edited

Loading

ghost commented Nov 28, 2017 •

edited by ghost

Loading

kamilogorek commented Nov 28, 2017 •

edited

Loading