search

dmanjunath / node-redshift

A simple collection of tools to help you get started with Amazon Redshift from node.js
69 stars 48 forks source link

Redshift errors #15

Closed Noursounet closed 7 years ago

Noursounet commented 7 years ago

Hi @dmanjunath ,

Me again :)

Just a quick question/remark about the way node-redshift npm handles Redshift errors: In query.js, you reject the promise this way:

reject(new Error(err));

Unfortunately we loose the RedShift error number.

Is there a reason why you have chosen to "override" the native RedShift error?

Thanks a lot for your answer!

Nours

dmanjunath commented 7 years ago

Hey again! Thanks for bringing this up. It's actually a bug so thank you for pointing it out. What happened was when I was promise-ifying with bluebird, I wasn't getting the full reject stacktrace, just bluebird's rejection. I attempted to fix it by creating an error at the query level, so at least I'd know where the promise was rejected instead of some random bluebird file. But, as you pointed out, you do lose information by doing it this way.

However, I have since figured out a better way to do this that preserves all the information. Bluebird has an option for longStackTraces(http://bluebirdjs.com/docs/api/promise.longstacktraces.html), which preserves the error with the trace of the calling file and function(I've bolded the stacktrace that I'd be looking for while debugging for example):

{ error: relation "ASDFASDF" does not exist at Connection.parseE (\~/node-redshift/node_modules/pg/lib/connection.js:554:11) at Connection.parseMessage (\~/node-redshift/node_modules/pg/lib/connection.js:381:17) at Socket. (\~/node-redshift/node_modules/pg/lib/connection.js:117:22) at emitOne (events.js:96:13) at Socket.emit (events.js:188:7) at readableAddChunk (_stream_readable.js:176:18) at Socket.Readable.push (_stream_readable.js:134:10) at TCP.onread (net.js:548:20) From previous event: at Redshift.query (\~/node-redshift/lib/query.js:32:12) at Object. (\~/node-redshift/test.js:104:24) at Module._compile (module.js:570:32) at Object.Module._extensions..js (module.js:579:10) at Module.load (module.js:487:32) at tryModuleLoad (module.js:446:12) at Function.Module._load (module.js:438:3) at Module.runMain (module.js:604:10) at run (bootstrap_node.js:394:7) at startup (bootstrap_node.js:149:9) at bootstrap_node.js:509:3 name: 'error', length: 99, severity: 'ERROR', code: '42P01', detail: undefined, hint: undefined, position: '15', internalPosition: undefined, internalQuery: undefined, where: undefined, schema: undefined, table: undefined, column: undefined, dataType: undefined, constraint: undefined, file: 'parse_relation.c', line: '984', routine: 'parserOpenTable' }

I'm thinking about adding an option that people can choose to set the longStackTraces to on or off. What do you think @Noursounet?

Noursounet commented 7 years ago

You mean besides the rawConnection option we would specify 'longStackTraces: true" and catch redshift full error details?

Yes that's perfect and would be of great help.

Thanks !

dmanjunath commented 7 years ago

@Noursounet You got it! I'll patch and push up a new npm version today

dmanjunath commented 7 years ago

Released