[231] Add Source Map support #565

andy-root · 2024-06-25T20:34:24Z

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

andy-root · 2024-06-25T21:54:44Z

andy-root · 2024-06-25T22:04:51Z

Apply source maps to error stacks. Enabling this feature will make fetch calls for javascript source and map files. If these files are cross domain then CORS headers must be included in their responses. If credentials or other headers are required then an optional fetch function is included.

Error events will get handled by plugins that handle errors, these include Fetch, Error and Xhr. Before each plugin records an event it will convert the Error Event to a JS Error Event. This error processing is were the Rum Event is created and the error stack is truncated. This makes for a good location to process the error stack. If the RUM client config has sourceMapsEnabled then the error's stack will be parsed and replaced with a stack written from source maps.

williazz · 2024-06-26T21:50:15Z

src/plugins/event-plugins/__tests__/JsErrorPlugin.test.ts

@@ -674,6 +719,69 @@ describe('JsErrorPlugin tests', () => {
        plugin.disable();

        // Assert
+        await waitForTick();


Question: thanks for adding these. Were these causing test flakiness?

This was needed because the tests were finishing before the record method has called resulting in failed tests. This is a result of changing recordHttpEventWithError() to an async method. This was the recommend way I found on jest to wait for async methods to finish. https://jestjs.io/docs/tutorial-async

williazz · 2024-06-26T21:52:15Z

src/plugins/utils/js-error-utils.ts

+                ajax: fetchFunction
+            } as StackTrace.StackTraceOptions);
+            error.stack = stackFrames.join(' \n ');
+            console.log('appendErrorSourceMaps done');


Nit: please delete the console.logs

missed one, ok

williazz · 2024-06-26T22:00:31Z

src/plugins/utils/js-error-utils.ts

+    if (error && error.stack) {
+        try {
+            const stackFrames = await StackTrace.fromError(error, {
+                ajax: fetchFunction


Issue: What does this ajax option do? I don't see it in documentation.

export interface StackTraceOptions { filter?: (stackFrame: StackFrame) => boolean; sourceCache?: SourceCache; offline?: boolean; }

https://github.com/stacktracejs/stacktrace.js/blob/710bba1118d396466ee342a30b3dfd19ecbda8b5/stacktrace-js.d.ts#L12-L16

Question: assuming ajax option works as intended, does that mean an http request is made on every single JSError? If so, is the result cached?

Question: how does stacktrace-js identify local source maps?

stacktrace-js uses stacktrace-gps to make the fetch calls. The options object is passed directly to new StackTraceGPS(), https://github.com/stacktracejs/stacktrace.js/blob/master/stacktrace.js#L104 stacktrace-gps takes ajax as an option, https://github.com/stacktracejs/stacktrace-gps/blob/master/README.md

From the stacktrace-gps documentation,

ajax: Function (String URL => Promise(responseText)) - Function to be used for making X-Domain requests

This is an optional fetch function that can be configured to send credentials or other headers when making requests for the js and map files.

When an error is processed fetch calls for js files are made for all the urls listed in the error stack, at the end of the file there should be a link to a map file, this is also fetched. I did not notice refetching of these files again by the browser if the same error occurs.

williazz · 2024-06-26T22:35:13Z

src/plugins/utils/js-error-utils.ts

+) => {
+    if (error && error.stack) {
+        try {
+            const stackFrames = await StackTrace.fromError(error, {


Nit: does this need its own try catch logic to capture validation errors?

From documentation: https://github.com/stacktracejs/stacktrace.js?tab=readme-ov-file#get-stack-trace-from-an-error

var error = new Error('BOOM!'); StackTrace.fromError(error).then(callback).catch(errback); //===> Promise(Array[StackFrame], Error) ``

I added this in the case that if an error does occur from the stacktrace lib that it gets recorded.

Suggestion: if there are validation errors from stacktrace-js, then the JSErrors look like they will be dropped. We may want to record the original error instead as a backup.

This try catch is here as a precaution is case StackTrace.fromError() fails, we would want to know so the catch error is added to the incoming error's stack . Whether StackTrace.fromError() fails or not, incoming errors are never dropped, the try catch is only attempts at replacing the incoming error's stack value, it does not return anything. We would not want to report the catch error as a normal error because it would create an infinite loop, the try catch helps avoid an error reporting infinite loop.

williazz · 2024-06-26T22:59:52Z

src/orchestration/Orchestration.ts

+     * required then provide a configured fetch function that returns the response as
+     * a string promise.
+     */
+    sourceMapsEnabled?: boolean;


Issue: to clarify, does this PR actually do two things?

Enhance stack traces of all JSErrors by taking on dependency stacktrace-js

Optionally enable source maps if sourceMapsEnabled and sourceMapsFetchFunction are supplied.

Question: How are the source maps retrieved if sourceMapsFetchFunction is undefined? If supplying sourceMapsFetchFunction is required to enable this feature, then sourceMapsEnabled may be redundant?

Setting sourceMapsEnabled to true is all that is needed unless additional headers or credentials are needed when requesting the js and map file. sourceMapsFetchFunction is optional, users would only provide a fetch method if they need to provide additional headers or credential when the stacktrace lib requests the js and map files.

Nit / backlog item (blocking): Before merging, we need to make it clear that this is a client side implementation maps: that is, update configuration names, pr title / description, and update documentation outlining the performance tradeoffs.

RUM service team also needs to make sure the names of your new feature do not clash with any server-side implementation in the future.

No action needed, we can clean it up

williazz · 2024-06-26T23:07:05Z

src/test-utils/test-utils.ts

@@ -110,6 +111,9 @@ export const getSession: jest.MockedFunction<GetSession> = jest.fn(() => ({
    eventCount: 0
 }));

+export const sourceMapsFetchFunction: jest.MockedFunction<SourceMapsFetchFunction> =


Backlog item: this needs an integration test to validate that source maps are being applied correctly

Yes, I stoped development and shared the PR so that RUM could decide if this client solution is something they want to move forward with vs. a service side solution or maybe this is a temporary solution? If we want to move forward I will work on the integration tests.

williazz · 2024-06-26T23:08:17Z

Thanks so much for the awesome PR! I left a few high level questions and can definitely help close the gaps.

williazz · 2024-06-27T01:05:52Z

Of the three fatures:

Enhance all JSErrors with stacktrace-js - this seems very close to mergable. We need better documentation on the expected diff and value add. Also we need to validate things such as package size increase; and how large JSErrors will become with the additional info since there are hard backend limits.
Apply local source maps - I don't understand if this is supported out of the box. If it is, then I have the same feedback as (1).
Fetch remote source maps - This needs more design based on the above comments, and probably should be a separate PR

andy-root · 2024-06-28T01:09:19Z

Comments on the Three features

For questions about local vs remote source maps, do you mean linked vs. inline? Minified js files will usually have source maps either linked at the end of the js file or the source map content will be included at the end of the js file. You either have 2 files, file.js and file.js.map or 1 larger js file that contains both the source and maps. In either case the change in size of the error stack will be the same.

FYI, I have our production apps using a similar design for applying sourcemaps with RUM and I increased our stacktraces from the default 1000 to 2000 through the RUM config.

[231] Add Source Map support

62a85ac

williazz self-requested a review June 26, 2024 21:35

williazz reviewed Jun 26, 2024

View reviewed changes

Remove console log statements

95c10cd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[231] Add Source Map support #565

[231] Add Source Map support #565

andy-root commented Jun 25, 2024

andy-root commented Jun 25, 2024

andy-root commented Jun 25, 2024

williazz Jun 26, 2024

andy-root Jun 28, 2024

williazz Jun 26, 2024

andy-root Jun 28, 2024

williazz Jun 26, 2024

williazz Jun 26, 2024

williazz Jun 26, 2024

andy-root Jun 28, 2024

williazz Jun 26, 2024 •

edited

Loading

andy-root Jun 28, 2024

williazz Jun 28, 2024

andy-root Jul 1, 2024 •

edited

Loading

williazz Jun 26, 2024

andy-root Jun 28, 2024

williazz Jul 3, 2024

williazz Jul 3, 2024

williazz Jun 26, 2024

andy-root Jun 28, 2024

williazz commented Jun 26, 2024

williazz commented Jun 27, 2024

andy-root commented Jun 28, 2024 •

edited

Loading

[231] Add Source Map support #565

Are you sure you want to change the base?

[231] Add Source Map support #565

Conversation

andy-root commented Jun 25, 2024

andy-root commented Jun 25, 2024

andy-root commented Jun 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williazz Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-root Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williazz commented Jun 26, 2024

williazz commented Jun 27, 2024

andy-root commented Jun 28, 2024 • edited Loading

williazz Jun 26, 2024 •

edited

Loading

andy-root Jul 1, 2024 •

edited

Loading

andy-root commented Jun 28, 2024 •

edited

Loading