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.

Sign up for GitHub

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

[231] Add Source Map support #565

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[231] Add Source Map support #565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
62a85ac
[231] Add Source Map support
Jun 25, 2024
95c10cd
Remove console log statements
Jul 3, 2024
d277da4
Merge branch 'aws-observability:main' into main
andy-root Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 90 additions & 6 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,9 @@
"@aws-sdk/signature-v4": "^3.36.0",
"@aws-sdk/util-hex-encoding": "^3.36.0",
"@babel/runtime": "^7.16.0",
"@types/stacktrace-js": "^2.0.3",
"shimmer": "^1.2.1",
"stacktrace-js": "^2.0.2",
"ua-parser-js": "^1.0.33",
"uuid": "^9.0.0",
"web-vitals": "^3.0.2"
Expand Down
14 changes: 14 additions & 0 deletions src/orchestration/Orchestration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,11 @@ export type CookieAttributes = {

export type PartialCookieAttributes = Partial<CookieAttributes>;

export type SourceMapsFetchFunction = (
input: RequestInfo,
init?: RequestInit
) => Promise<string>;

export interface Config {
allowCookies: boolean;
batchLimit: number;
Expand Down Expand Up @@ -159,6 +164,15 @@ export interface Config {
telemetries: Telemetry[];
useBeacon: boolean;
userIdRetentionDays: number;
/**
* 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 provide a configured fetch function that returns the response as
* a string promise.
*/
sourceMapsEnabled?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Enhance stack traces of all JSErrors by taking on dependency stacktrace-js
  2. 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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No action needed, we can clean it up

sourceMapsFetchFunction?: SourceMapsFetchFunction;
}

export interface PartialConfig
Expand Down
12 changes: 7 additions & 5 deletions src/plugins/event-plugins/FetchPlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,16 +250,18 @@ export class FetchPlugin extends MonkeyPatched<Window, 'fetch'> {
}
};

private recordHttpEventWithError = (
private recordHttpEventWithError = async (
httpEvent: HttpEvent,
error: Error | string | number | boolean | undefined | null
) => {
httpEvent.error = errorEventToJsErrorEvent(
httpEvent.error = await errorEventToJsErrorEvent(
{
type: 'error',
error
} as ErrorEvent,
this.config.stackTraceLength
this.config.stackTraceLength,
this.context.config.sourceMapsEnabled,
this.context.config.sourceMapsFetchFunction
);
this.context.record(HTTP_EVENT_TYPE, httpEvent);
};
Expand Down Expand Up @@ -295,9 +297,9 @@ export class FetchPlugin extends MonkeyPatched<Window, 'fetch'> {
this.recordHttpEventWithResponse(httpEvent, response);
return response;
})
.catch((error: Error) => {
.catch(async (error: Error) => {
this.endTrace(trace, undefined, error);
this.recordHttpEventWithError(httpEvent, error);
await this.recordHttpEventWithError(httpEvent, error);
throw error;
});
};
Expand Down
9 changes: 7 additions & 2 deletions src/plugins/event-plugins/JsErrorPlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,15 @@ export class JsErrorPlugin extends InternalPlugin {
}
};

private recordJsErrorEvent(error: ErrorEvent) {
private async recordJsErrorEvent(error: ErrorEvent) {
this.context?.record(
JS_ERROR_EVENT_TYPE,
errorEventToJsErrorEvent(error, this.config.stackTraceLength)
await errorEventToJsErrorEvent(
error,
this.config.stackTraceLength,
this.context.config.sourceMapsEnabled,
this.context.config.sourceMapsFetchFunction
)
);
}

Expand Down
8 changes: 5 additions & 3 deletions src/plugins/event-plugins/XhrPlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ export class XhrPlugin extends MonkeyPatched<XMLHttpRequest, 'send' | 'open'> {
}
}

private recordHttpEventWithError(
private async recordHttpEventWithError(
xhrDetails: XhrDetails,
xhr: XMLHttpRequest,
error: Error | string | number | boolean | undefined | null
Expand All @@ -278,12 +278,14 @@ export class XhrPlugin extends MonkeyPatched<XMLHttpRequest, 'send' | 'open'> {
const httpEvent: HttpEvent = {
version: '1.0.0',
request: { method: xhrDetails.method, url: xhrDetails.url },
error: errorEventToJsErrorEvent(
error: await errorEventToJsErrorEvent(
{
type: 'error',
error
} as ErrorEvent,
this.config.stackTraceLength
this.config.stackTraceLength,
this.context.config.sourceMapsEnabled,
this.context.config.sourceMapsFetchFunction
)
};
if (this.isTracingEnabled()) {
Expand Down
Loading