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

🐛 AddError should support all instances of type Error #3228

Conversation

lightswitch05
Copy link
Contributor

@lightswitch05 lightswitch05 commented Dec 18, 2024

Motivation

Recently, all errors of type AxiosError stopped logging properly. Instead of the standard log with Error object, it was serializing the error object instead.

After some investigation I tracked down the regression to #3144 - which changes the object type check from param instanceof Error to Object.prototype.toString.call(error) === '[object Error]'. Unfortunately, the way AxiosError is created, that toString call just produces [object Object]. However, the instanceof check works just fine.

Image 12-17-24 at 6 03 PM

Changes

I've addressed the regression by using both the original instanceof Error check as well as the new toString check. Supporting both checks cover both the old and the new feature requirements.

Testing

I spent some time trying to get the tests running - but I was not successful. I don't doubt there might be some tweaks required in the PR, but I think the bulk of it is complete. Please feel free to modify the PR as needed. Hopefully this can get fixed quickly and we can all have AxiosError logging working again.

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@lightswitch05 lightswitch05 requested a review from a team as a code owner December 18, 2024 00:01
@bits-bot
Copy link

bits-bot commented Dec 18, 2024

CLA assistant check
All committers have signed the CLA.

@lightswitch05 lightswitch05 changed the title AddError should support all instances of type Error 🐛 AddError should support all instances of type Error Dec 18, 2024
@thomas-lebeau thomas-lebeau added the bug Something isn't working label Dec 18, 2024
@RomanGaignault
Copy link
Contributor

/to-staging

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Dec 18, 2024

Devflow running: /to-staging

View all feedbacks in Devflow UI.


2024-12-18 15:23:41 UTC ℹ️ Branch Integration: starting soon, median merge time is 0s

Commit 26edcbf7d0 will soon be integrated into staging-51.


2024-12-18 15:27:46 UTC 🚨 Branch Integration: The build pipeline contains failing jobs for this merge request

We couldn't automatically merge the commit 26edcbf7d0 into staging-51.
Build pipeline has failing jobs for bb4ac3b:

⚠️ Do NOT retry failed jobs directly (why?).

What to do next?

  • Investigate the failures and when ready, re-add your pull request to the queue!
  • Any question, go check the FAQ.

If you think the errors come from a logical conflict with the target branch, you can create a fix by commenting this pull request with /create-fix-branch -b staging-51

Details

Since those jobs are not marked as being allowed to fail, the pipeline will most likely fail.
Therefore, and to allow other builds to be processed, this merge request has been rejected and the pipeline got canceled.

@RomanGaignault RomanGaignault merged commit 166489d into DataDog:main Dec 18, 2024
22 checks passed
@RomanGaignault
Copy link
Contributor

RomanGaignault commented Dec 18, 2024

@lightswitch05 the pr is merged, we will do a release soon 😃

@lightswitch05
Copy link
Contributor Author

image

Just looping back to confirm AxiosError logging is back! Thank you for the quick response! 💪 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants