fix(instrumentation-knex): Support better-sqlite3 errors

[AbhiPrasad]

Which problem is this PR solving?

resolves #2297

Short description of the changes

Make sure better-sqlite3 is supported by not assuming how the error constructor is set up. Adds test accordingly.

[AbhiPrasad]

To constrain this PR I only added this change here. In a follow up PR after this merges, I want to refactor this file so that we can do a matrix test against multiple clients.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.30%. Comparing base (dfb2dff) to head (c2eb363).
Report is 173 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2298      +/-   ##
==========================================
- Coverage   90.97%   90.30%   -0.67%     
==========================================
  Files         146      147       +1     
  Lines        7492     7264     -228     
  Branches     1502     1509       +7     
==========================================
- Hits         6816     6560     -256     
- Misses        676      704      +28     

Files	Coverage Δ
...de/opentelemetry-instrumentation-knex/src/utils.ts	90.47% <100.00%> (+0.23%)	⬆️

... and 56 files with indirect coverage changes

[pichlermarc]

Looks great, thx 🙂

[trentm]

What do you think about the following change:

diff --git a/plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts
index c80ebbe5..d539126f 100644
--- a/plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts
+++ b/plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts
@@ -176,8 +176,8 @@ export class KnexInstrumentation extends InstrumentationBase {
             const formatter = utils.getFormatter(this);
             const fullQuery = formatter(query.sql, query.bindings || []);
             const message = err.message.replace(fullQuery + ' - ', '');
-            const clonedError = utils.cloneErrorWithNewMessage(err, message);
-            span.recordException(clonedError);
+            const exc = utils.otelExceptionFromKnexError(err, message);
+            span.recordException(exc);
             span.setStatus({ code: api.SpanStatusCode.ERROR, message });
             span.end();
             throw err;
diff --git a/plugins/node/opentelemetry-instrumentation-knex/src/utils.ts b/plugins/node/opentelemetry-instrumentation-knex/src/utils.ts
index 12ba70ed..e3c82f78 100644
--- a/plugins/node/opentelemetry-instrumentation-knex/src/utils.ts
+++ b/plugins/node/opentelemetry-instrumentation-knex/src/utils.ts
@@ -14,17 +14,14 @@
  * limitations under the License.
  */
 
+import { Exception } from '@opentelemetry/api';
 import {
   DBSYSTEMVALUES_SQLITE,
   DBSYSTEMVALUES_POSTGRESQL,
 } from '@opentelemetry/semantic-conventions';
 
-type Exception = {
-  new (message: string): Exception;
-  constructor: Exception;
-  errno?: number;
+type KnexError = Error & {
   code?: string;
-  stack?: string;
 };
 
 export const getFormatter = (runner: any) => {
@@ -43,16 +40,17 @@ export const getFormatter = (runner: any) => {
   return () => '<noop formatter>';
 };
 
-export const cloneErrorWithNewMessage = (err: Exception, message: string) => {
-  if (err && err instanceof Error) {
-    const clonedError = Object.assign({}, err);
-    clonedError.message = message;
-    clonedError.code = err.code;
-    clonedError.stack = err.stack;
-    clonedError.errno = err.errno;
-    return clonedError;
+export function otelExceptionFromKnexError(err: KnexError, message: string): Exception {
+  if (!(err && err instanceof Error)) {
+    return err;
   }
-  return err;
+
+  return {
+    message,
+    code: err.code,
+    stack: err.stack,
+    name: err.name,
+  };
 };
 
 const systemMap = new Map([

This clarifies that the utility function is about returning an object to be used in span.recordException, i.e. type Exception (https://github.com/open-telemetry/opentelemetry-js/blob/main/api/src/common/Exception.ts). Benefits:

• It doesn't bother copying the errno field, which wasn't being used. (If Knex errors can include an errno instead of a 'code', we migth want to consider doing something with the errno field.) It also doesn't bother copying other err fields, if any.
• It also includes err.name, so there is a fallback value for the exception.type attribute if err.code is not defined. Otherwise Span#recordException can fail to record the exception: https://github.com/open-telemetry/opentelemetry-js/blob/01a2c35a694e57df45f063d61506ef9e9938eb7d/packages/opentelemetry-sdk-trace-base/src/Span.ts#L308-L329

This could all also be done in a separate change. I'm happy to PR that separately if you prefer.

[trentm]

Note that knex@3 does drop support for node 14. Currently the Node.js v14 unit-tests passed, however.
I am more inclined to be testing with the recent knex than locking at the old version. There may come a knex 3.x version that breaks our Node.js v14 tests.

[trentm]

@AbhiPrasad I think this could be merged as is, given the approvals. A polite ping on my question above: #2298 (comment)

[trentm]

@AbhiPrasad Also, are you able to update? Your fork repo doesn't allow commits from me so I cannot update the base branch.

[jmadureira]

@AbhiPrasad and @trentm any news on this PR? Anything I can help with?