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

feat(node): Improve non-error messages #9026

Merged
merged 2 commits into from
Sep 15, 2023

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Sep 14, 2023

Closes #6822

This PR changes the messages reported for captured non-Error exceptions.

For the browser PR, it was decided not to use the class name due to these being mangled by bundling. Does this apply to node too?

Copy link
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

This is def. an improvement already. I guess it is less likely to have minified class names in node, but still possible? But IMHO we can merge this in and still think about making this better for node if possible.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

it was decided to to use the class name due to these being mangled by bundling. Does this apply to node too?

yes, since node can be minified (common for serverless) and minified js has performance benefits even for server runtimes.

As an aside, I wonder if we can detect if object has an name, message, and stack and turn it into an error. Thoughts?

@mydea
Copy link
Member

mydea commented Sep 14, 2023

it was decided to to use the class name due to these being mangled by bundling. Does this apply to node too?

yes, since node can be minified (common for serverless) and minified js has performance benefits even for server runtimes.

As an aside, I wonder if we can detect if object has an name, message, and stack and turn it into an error. Thoughts?

I guess it would already be a possible improvement to just detect name and/or message and use these, if available?

@timfish
Copy link
Collaborator Author

timfish commented Sep 14, 2023

As an aside, I wonder if we can detect if object has an name, message, and stack and turn it into an error. Thoughts?

Ok, I'll add that and some tests.

It worth noting that it already attempts to parse the stack property for any kind of object.

@AbhiPrasad AbhiPrasad merged commit 4a4366d into getsentry:develop Sep 15, 2023
68 checks passed
billyvg pushed a commit that referenced this pull request Sep 15, 2023
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.

"Non-Error exception captured with keys: [...]" due to invalid error instanceof check
3 participants