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

Include original error stack trace for Rollbar when throwing error in block pipeline #2973

Closed
twschiller opened this issue Mar 20, 2022 · 7 comments · Fixed by #3011
Closed
Assignees

Comments

@twschiller
Copy link
Contributor

twschiller commented Mar 20, 2022

Throw block errors reported to Rollbar needs to include original stack trace. Otherwise for application errors we can't pinpoint what line caused the error

image

Relevant code:

Related:

@fregante
Copy link
Collaborator

fregante commented Mar 20, 2022

Indeed, we discussed this before I think:

I'll re-read your long comment in that last thread as it contains a lot information about a setup that we haven't fully achieved yet.

Related:

Do you think I should work on this so we don't have to worry about serialized errors anymore?

Also:

@fregante
Copy link
Collaborator

I looked into a few solutions:

  • Error#cause is already used by ContextError but it doesn't appear to be followed/displayed by Rollbar
  • the make-error-cause module seemed to fit but actually just concatenates the stacks
  • AggregateError is a new native error group, but unfortunately the console does not show its "children errors" yet and likely neither does Rollbar
  • the aggregate-error module seemed to offer a clean API to merge errors and their stacks, but its stack merger/cleaner isn't optimized outside Node

Here's for example what the result of aggregate-error looks like:

Screen Shot 2

So I can suggest just appending the stack like everyone else seems to be doing, something like:

try {
	throw new Error('Original')
} catch (original) {
	const composed = new Error('New details: ' + original.message)
	composed.stack += '\n' + original.stack
	throw composed;
}

and it results into:

Screen Shot 3

Pros:

  • no lost details (including each error Type)
  • the last-thrown error message contains all the information

I'll turn this pattern into a helper so we can create such wrappers easily, like:

class ContextError extend WrappedError {
	constructor(message, cause, context) {
		super(message, cause); // Will automatically concatenate the message and stacks
		this.context = context;
	}
}

@fregante
Copy link
Collaborator

fregante commented Mar 21, 2022

Additionally, for the long term, we'd want Error#cause support in:

@twschiller
Copy link
Contributor Author

twschiller commented Mar 24, 2022

Additionally, for the long term, we'd want Error#cause support in:

  • Rollbar (maybe you can open a ticket?)

Opened a ticket with rollbar: rollbar/rollbar.js#1010

A workaround might be to overload the payload transform method? (Which is where we currently re-write the host on the stack track URLS) Although likely the frames from the cause likely don't make their way to the the payload? With your ErrorWithCause though, we might not need to?

@cyrusradfar
Copy link

@fregante @twschiller hi, I do Product at Rollbar. I've followed up on the thread in rollbar/rollbar.js

@fregante
Copy link
Collaborator

@twschiller great idea! Today I'll see if Rollbar supports the current concatenated stack and if it does I'll move the concatenation logic to that transform. 🎉

@fregante
Copy link
Collaborator

fregante commented Apr 8, 2022

Update:

  • Rollbar already had (undocumented) support for nested errors using the property nested: (below: b is the cause)

Screen Shot

  • They just added the same support for cause as well, but it hasn't been shipped yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants