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

✨ [RUMF-922] stack trace on handled calls #889

Merged
merged 12 commits into from
Jun 17, 2021

Conversation

amortemousque
Copy link
Contributor

@amortemousque amortemousque commented Jun 10, 2021

Motivation

Some errors are reported without a proper Error object. In these cases, there is no stack trace reported, making them a lot less actionable by the user. In order to enrich these errors, RUM need to generate a stack trace internally.

Changes

  • Generate an handling stack on console.log
  • Generate an handling stack on addError

Testing

Unit, Locally


I have gone over the contributing documentation.

@amortemousque amortemousque changed the title ✨ stack trace on instrumentation calls ✨ [RUMF-922] stack trace on instrumentation calls Jun 10, 2021
@amortemousque amortemousque force-pushed the aymeric/stacktrace-instrumentation-call branch from 11d3d24 to 01d9fba Compare June 10, 2021 10:03
@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2021

Codecov Report

Attention: Patch coverage is 90.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 89.09%. Comparing base (9cf758d) to head (eb44532).
Report is 1710 commits behind head on main.

Files with missing lines Patch % Lines
packages/core/src/tools/error.ts 76.92% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #889      +/-   ##
==========================================
- Coverage   89.13%   89.09%   -0.04%     
==========================================
  Files          81       81              
  Lines        3809     3825      +16     
  Branches      850      851       +1     
==========================================
+ Hits         3395     3408      +13     
- Misses        414      417       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@amortemousque amortemousque force-pushed the aymeric/stacktrace-instrumentation-call branch from 01d9fba to 972ce8e Compare June 10, 2021 12:27
@amortemousque amortemousque force-pushed the aymeric/stacktrace-instrumentation-call branch 3 times, most recently from 90ceb76 to 59b0aa4 Compare June 14, 2021 10:08
@amortemousque amortemousque marked this pull request as ready for review June 14, 2021 11:42
@amortemousque amortemousque requested a review from a team as a code owner June 14, 2021 11:42
@amortemousque amortemousque changed the title ✨ [RUMF-922] stack trace on instrumentation calls ✨ [RUMF-922] stack trace on handled calls Jun 14, 2021
@amortemousque amortemousque force-pushed the aymeric/stacktrace-instrumentation-call branch from 59b0aa4 to c7ec309 Compare June 14, 2021 13:29
packages/core/src/tools/error.spec.ts Outdated Show resolved Hide resolved
packages/core/src/tools/error.ts Outdated Show resolved Hide resolved
packages/core/src/tools/error.ts Outdated Show resolved Hide resolved
packages/core/src/tools/error.ts Outdated Show resolved Hide resolved
packages/core/src/tools/error.ts Outdated Show resolved Hide resolved
packages/core/src/tools/error.ts Outdated Show resolved Hide resolved
packages/core/src/tools/error.ts Outdated Show resolved Hide resolved
packages/core/src/tools/error.ts Outdated Show resolved Hide resolved
packages/core/src/domain/automaticErrorCollection.spec.ts Outdated Show resolved Hide resolved
packages/core/src/domain/automaticErrorCollection.spec.ts Outdated Show resolved Hide resolved
packages/core/src/tools/error.spec.ts Outdated Show resolved Hide resolved
packages/core/src/tools/error.spec.ts Outdated Show resolved Hide resolved
it('should get handling stack trace without instrumental function calls', () => {
userCallOne()

const formattedStackTrace = toStackTraceString(stackTrace)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of coupling handling stack tests with the implementation of toStackTraceString, what about directly returning a stack trace string from createHandlingStackTrace?
it would also avoid callers to do this conversion later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about it. I tried to be consistent with the way we handle the "normal" stack trace.

function buildErrorFromParams(params: unknown[], handlingStack: StackTrace) {
const firstErrorParam = find(params, (param: unknown): param is Error => param instanceof Error)
return {
message: ['console error:', ...params].map((param) => formatConsoleParameters(param)).join(' '),
stack: firstErrorParam ? toStackTraceString(computeStackTrace(firstErrorParam)) : undefined,
handlingStack: toStackTraceString(handlingStack),
}
}

Also, by looking at error.ts, it seems that the idea was to keep stack traces as StackTrace type and to be able to make transformation on them.
export function formatUnknownError(
stackTrace: StackTrace | undefined,
errorObject: any,
nonErrorPrefix: string,
handlingStack?: StackTrace

Maybe I should change the test to only test the stack trace as StackTrace type and not as string. It would also make sens because the toStackTraceString has already been tested.

Copy link
Contributor

Choose a reason for hiding this comment

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

StackTrace is a tracekit type, it seems ok to me to keep it for tracekit API output but I'd be in favor of avoiding to use it internally if we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay done

packages/rum-core/src/boot/rumPublicApi.spec.ts Outdated Show resolved Hide resolved
@amortemousque amortemousque force-pushed the aymeric/stacktrace-instrumentation-call branch from 6105755 to e2bb68c Compare June 16, 2021 10:26
@amortemousque amortemousque force-pushed the aymeric/stacktrace-instrumentation-call branch from e2bb68c to a9f74a3 Compare June 16, 2021 10:31
@amortemousque amortemousque merged commit e7fdae5 into main Jun 17, 2021
@bcaudan bcaudan deleted the aymeric/stacktrace-instrumentation-call branch September 13, 2021 08:08
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.

3 participants