fix(instr-knex): handle config provider functions

[reberhardt7]

Which problem is this PR solving?

The knex connection configuration can be set with a static object, or with a function. From the knex docs:

A function can be used to determine the connection configuration dynamically. This function receives no parameters, and returns either a configuration object or a promise for a configuration object.

const knex = require('knex')({
  client: 'sqlite3',
  connection: () => ({
    filename: process.env.SQLITE_FILENAME,
  }),
});

This instrumentation didn't handle the function case previously, resulting in undefined in the span name and a bunch of missing attributes.

Short description of the changes

Instead of accessing properties in client.config.connection, the instrumentation should be accessing client.connectionSettings. This gets set for static configs here and for config provider functions here

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.30%. Comparing base (dfb2dff) to head (3fcb711).
Report is 170 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2286      +/-   ##
==========================================
- 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 Δ
...emetry-instrumentation-knex/src/instrumentation.ts	98.71% <100.00%> (-0.07%)	⬇️

... and 60 files with indirect coverage changes

[reberhardt7]

I've been getting test failures caused by network flakes:

npm ERR! code ECONNRESET
npm ERR! network aborted
npm ERR! network This is a problem related to network connectivity.
npm ERR! network In most cases you are behind a proxy or have bad network settings.
npm ERR! network
npm ERR! network If you are behind a proxy, please make sure that the
npm ERR! network 'proxy' config is set properly. See: 'npm help config'

If someone with permissions could rerun the tests for me, that would be much appreciated

[blumamir]

LGTM.

Thank you for contributing a fix to this issue, and also for adding useful links to provide more context for the review.

I do have one question - since the connection can be an async function that can resolves sometime in the future, is it possible that if a quesy is made very early, that the connectionSettings are still not resolved? Do you think it's worth adding a test for this case?

[reberhardt7]

@blumamir I just tried writing a test for this case:

  it('works when knex query is made before connection function resolves', async () => {
    let resolveConfig: () => void;
    const configResolved = new Promise<void>((resolve) => {
      resolveConfig = resolve;
    })
    client = knex({
      client: 'sqlite3',
      // Use a function to configure the connection
      connection: async () => {
        await configResolved;
        return {
          filename: ':memory:',
        };
      },
      useNullAsDefault: true,
    });

    const queryResult = client.raw('SELECT 1');
    process.nextTick(resolveConfig!);

    await queryResult;
    const instrumentationSpans = memoryExporter.getFinishedSpans();
    assert.strictEqual(instrumentationSpans.length, 1);
    assert.strictEqual(instrumentationSpans[0].name, 'raw :memory:');
    assert.strictEqual(
      instrumentationSpans[0].attributes['net.transport'],
      'inproc'
    );
  })

It does pass, as I believe knex must resolve the config function before attempting to make any queries. However, without being able to hook into the implementation of knex, this test seems inherently racey, since there is no way to guarantee that we start trying to run the query before resolving the config function. Do you think it is still worth adding?

[reberhardt7]

@blumamir What are the next steps for trying to land this PR?