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

✨ [RUM-2902] Add error causes to context when logging an error #2602

Merged
merged 8 commits into from
Feb 22, 2024

Conversation

N-Boutaib
Copy link
Contributor

@N-Boutaib N-Boutaib commented Feb 12, 2024

Motivation

This PR addresses this issue in order to add an error's causes when logging it.

Changes

Add flattened error causes to the error's context when logging it.

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@N-Boutaib N-Boutaib changed the title ♻️ Rename error var :spakles: Add error causes to context when logging an error Feb 13, 2024
@N-Boutaib N-Boutaib changed the title :spakles: Add error causes to context when logging an error ✨ Add error causes to context when logging an error Feb 13, 2024
@N-Boutaib N-Boutaib changed the title ✨ Add error causes to context when logging an error ✨ [RUM-2902] Add error causes to context when logging an error Feb 13, 2024
@N-Boutaib N-Boutaib marked this pull request as ready for review February 13, 2024 09:49
@N-Boutaib N-Boutaib requested a review from a team as a code owner February 13, 2024 09:49
@N-Boutaib N-Boutaib self-assigned this Feb 13, 2024
@BenoitZugmeyer
Copy link
Member

PR looks good, but I think we should collect error causes for other errors collected by the Logs SDK:

I think that's it?

Maybe a function to format a rawError into log error.* properties could be nice, so we have cause, fingerprint and kind properly defined everywhere as well.

@codecov-commenter
Copy link

codecov-commenter commented Feb 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f6f76a1) 92.78% compared to head (c8609ec) 92.78%.
Report is 14 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2602   +/-   ##
=======================================
  Coverage   92.78%   92.78%           
=======================================
  Files         235      235           
  Lines        6794     6795    +1     
  Branches     1489     1490    +1     
=======================================
+ Hits         6304     6305    +1     
  Misses        490      490           

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

@amortemousque
Copy link
Contributor

💭 thought: ‏ It's not directly related to the PR but looking at how we collect errors in Logs it seems we have some discrepancies:

  • error.fingerprint only set in console.error but not in logger.error
  • error.message only set in logger.error but not in other error collections
  • error.kind not set in console.error

It could be nice to have a more centralize ways to format Error. You could introduce something in this PR like mentioned by @BenoitZugmeyer (cf comment) or maybe in a following PR :)

@N-Boutaib
Copy link
Contributor Author

/merge

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Feb 22, 2024

🚂 MergeQueue

Pull request added to the queue.

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

Use /merge -c to cancel this operation!

@dd-mergequeue dd-mergequeue bot merged commit bcb17e0 into main Feb 22, 2024
18 checks passed
@dd-mergequeue dd-mergequeue bot deleted the najib.boutaib/support-error-cause-in-rum-logs branch February 22, 2024 09:26
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