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-1508] reorganise error handling #2181

Merged
merged 10 commits into from
Apr 25, 2023
Merged

Conversation

bcaudan
Copy link
Contributor

@bcaudan bcaudan commented Apr 20, 2023

Motivation

Prepare handling of error without stacktrace

Changes

  • add more tests
  • reorganise instrumentOnError
  • reorganise computeRawError

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

Copy link
Contributor Author

bcaudan commented Apr 20, 2023

@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2023

Codecov Report

Merging #2181 (443f8d9) into main (9b61225) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2181      +/-   ##
==========================================
+ Coverage   93.81%   93.85%   +0.04%     
==========================================
  Files         201      201              
  Lines        6062     6073      +11     
  Branches     1337     1344       +7     
==========================================
+ Hits         5687     5700      +13     
+ Misses        375      373       -2     
Impacted Files Coverage Δ
packages/core/src/domain/error/error.ts 95.16% <100.00%> (+1.28%) ⬆️
packages/core/src/domain/tracekit/tracekit.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bcaudan bcaudan force-pushed the bcaudan/error-without-stack branch from ffbe29c to d8b82b4 Compare April 21, 2023 08:17
@bcaudan bcaudan force-pushed the bcaudan/error-without-stack branch 2 times, most recently from 53e2d9d to 22f22be Compare April 21, 2023 12:16
Base automatically changed from bcaudan/error-refacto to main April 21, 2023 12:16
@bcaudan bcaudan force-pushed the bcaudan/error-without-stack branch from 22f22be to 57f527c Compare April 21, 2023 12:19
@bcaudan bcaudan force-pushed the bcaudan/error-without-stack branch from 57f527c to d87f16e Compare April 21, 2023 15:32
@bcaudan bcaudan marked this pull request as ready for review April 24, 2023 07:45
@bcaudan bcaudan requested a review from a team as a code owner April 24, 2023 07:45
@bcaudan bcaudan force-pushed the bcaudan/error-without-stack branch from a433851 to 62e2d43 Compare April 24, 2023 14:12
@bcaudan
Copy link
Contributor Author

bcaudan commented Apr 24, 2023

/to-staging

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Apr 24, 2023

🚂 Branch Integration

commit 62e2d43 will soon be integrated into staging-17.

This build is going to start soon! (estimated merge in less than 0s)

you can cancel this operation by commenting your pull request with /to-staging -c!

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Apr 24, 2023

🚂 Branch Integration

commit 62e2d43 has been merged into staging-17

packages/core/src/domain/error/trackRuntimeError.spec.ts Outdated Show resolved Hide resolved
packages/core/src/domain/tracekit/tracekit.ts Show resolved Hide resolved
}
return stackTrace.stack.length > 0 && (stackTrace.stack.length > 1 || stackTrace.stack[0].url !== undefined)
Copy link
Member

Choose a reason for hiding this comment

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

❓ question: ‏I don't see this check in the original code, why do we need it now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are cases where stackTrace.stack = [] or stackTrace.stack = [{url: undefined, column: undefined, line: undefined}] and we want to use the NO_ERROR_STACK_PRESENT_MESSAGE for those.
In the original code, those cases happened in the first if but it will change in the following PR.

packages/core/src/domain/error/error.ts Outdated Show resolved Hide resolved
@bcaudan
Copy link
Contributor Author

bcaudan commented Apr 25, 2023

/to-staging

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Apr 25, 2023

🚂 Branch Integration

commit 443f8d9 will soon be integrated into staging-17.

This build is going to start soon! (estimated merge in less than 34m)

you can cancel this operation by commenting your pull request with /to-staging -c!

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Apr 25, 2023

🚂 Branch Integration

commit 443f8d9 has been merged into staging-17

@bcaudan bcaudan merged commit 0854beb into main Apr 25, 2023
@bcaudan bcaudan deleted the bcaudan/error-without-stack branch April 25, 2023 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants