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

Errors from Result will now contain the caller stack trace #303

Merged

Conversation

legraphista
Copy link
Contributor

Consider the following code:

const fn_a = cb => fn_b(cb);
const fn_b = cb => fn_c(cb);
const fn_c = cb => session.run('RETURN 1/0 AS x').catch(cb).then(_ => cb(void 0, _));

Before, when the execution had an error, we would get the following stack track:

Neo4jError: / by zero
    at new Neo4jError (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/error.js:76:132)
    at newError (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/error.js:66:10)
    at Connection._handleMessage (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/internal/connector.js:356:56)
    at Dechunker.Connection._dechunker.onmessage (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/internal/connector.js:287:12)
    at Dechunker._onHeader (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/internal/chunking.js:246:14)
    at Dechunker.AWAITING_CHUNK (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/internal/chunking.js:199:21)
    at Dechunker.write (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/internal/chunking.js:257:28)
    at NodeChannel.self._ch.onmessage (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/internal/connector.js:260:27)
    at TLSSocket.<anonymous> (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/internal/ch-node.js:308:16)
    at emitOne (events.js:115:13)

In this case is easy to pinpoint the issue, but in a broader environment where there are a lot of queries, like for example in an API endpoint, It's hard find the bad one, even harder if the error doesn't contain any cypher at all for you to try to match against.

This change stores the current calling stack trace in this._stack in the constructor of Result and concatenates it with the original stack trace of Result in cases when Result throws an error.

Neo4jError: / by zero
    at new Neo4jError (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/error.js:76:132)
    at newError (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/error.js:66:10)
    at Connection._handleMessage (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/internal/connector.js:356:56)
    at Dechunker.Connection._dechunker.onmessage (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/internal/connector.js:287:12)
    at Dechunker._onHeader (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/internal/chunking.js:246:14)
    at Dechunker.AWAITING_CHUNK (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/internal/chunking.js:199:21)
    at Dechunker.write (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/internal/chunking.js:257:28)
    at NodeChannel.self._ch.onmessage (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/internal/connector.js:260:27)
    at TLSSocket.<anonymous> (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/internal/ch-node.js:308:16)
    at emitOne (events.js:115:13)
    at new Result (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/result.js:73:19)
    at Session._run (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/session.js:122:14)
    at Session.run (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/session.js:101:19)
    at fn_c (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/test.js:18:39)
    at fn_b (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/test.js:12:23)
    at fn_a (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/test.js:11:23)
    at __dirname (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/test.js:30:9)
    at Object.<anonymous> (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/test.js:31:3)
    at Module._compile (module.js:624:30)
    at Object.Module._extensions..js (module.js:635:10)

With the new stack trace, the location of the issue is immediately available to the developer to look at and fix.

...
    at fn_c (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/test.js:18:39)
    at fn_b (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/test.js:12:23)
    at fn_a (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/test.js:11:23)
    at __dirname (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/test.js:30:9)
    at Object.<anonymous> (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/test.js:31:3)
...

Also included test for it and all the other test are passing.

Copy link

@eek eek left a comment

Choose a reason for hiding this comment

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

Definitely useful! 👍

@lutovich
Copy link
Contributor

lutovich commented Nov 6, 2017

Hi @legraphista,

Thanks for this PR. I definitely agree that having "real" stacktrace is valuable.
However, I think that concatenated stacktraces look weird. What do you think about just replacing the original stacktrace with Result._stack?

@legraphista
Copy link
Contributor Author

Hey @lutovich,

That was my first approach, but then you'd loose backwards compatibility with anything that was dependent on that stack trace. But if you'd like, I can update the branch to replace the original stack trace.

@lutovich
Copy link
Contributor

lutovich commented Nov 6, 2017

@legraphista do you think there might be some code out there that depends on the stacktrace?
I do not really know if it is common to parse stacktraces in real JS applications.

@legraphista
Copy link
Contributor Author

Yes @lutovich, it's uncommon to parse the stack traces, I was thinking more from a logging side of things. But I think you're right, the old stack traces don't offer you much in terms of locating the issue, might as well just fully replace them.

@legraphista
Copy link
Contributor Author

I've pushed the change, new stack trace example looks like:

Neo4jError: / by zero
    at new Result (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/result.js:73:19)
    at Session._run (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/session.js:122:14)
    at Session.run (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/lib/v1/session.js:101:19)
    at c (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/test.js:18:39)
    at b (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/test.js:12:23)
    at a (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/test.js:11:23)
    at __dirname (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/test.js:30:9)
    at Object.<anonymous> (/Users/Stefan/Desktop/GitHub/neo4j-javascript-driver/test.js:31:3)
    at Module._compile (module.js:624:30)
    at Object.Module._extensions..js (module.js:635:10)

@legraphista legraphista force-pushed the feature/1.5/stack-trace-outside-the-driver branch 2 times, most recently from 7271b55 to 87b644e Compare November 6, 2017 13:24
@lutovich
Copy link
Contributor

lutovich commented Nov 7, 2017

@legraphista thanks, change looks good to me.
@oskarhane could you please take a look as well?

@lutovich lutovich merged commit 035f675 into neo4j:1.5 Nov 8, 2017
@legraphista legraphista deleted the feature/1.5/stack-trace-outside-the-driver branch November 8, 2017 11:43
@lutovich
Copy link
Contributor

lutovich commented Nov 8, 2017

@legraphista merged, thanks again!
I think we'll release 1.5.1 version with this fix in a week or two.

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.

3 participants