fix(knex): connection attrs missing if connectionString used

[edosrecki]

Which problem is this PR solving?

• Connection related attributes (hostname, port, user, database) are missing if users use connectionString property when initializing knex (instead of specifying individual fields):

const pg = require('knex')({
  client: 'pg',
  connection: {
    connectionString: config.DATABASE_URL,
  },
});

• Span name is also affected, for example here database = undefined:

Short description of the changes

• If connectionString is provided in knex options, extract attributes from it (hostname, port, user, database).
• If both connectionString and individual fields are used, connectionString takes precedence:

[codecov]

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.02%. Comparing base (7895306) to head (d6e6090).
Report is 514 commits behind head on main.

Files with missing lines	Patch %	Lines
...emetry-instrumentation-knex/src/instrumentation.ts	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1932      +/-   ##
==========================================
- Coverage   91.02%   91.02%   -0.01%     
==========================================
  Files         146      146              
  Lines        7478     7488      +10     
  Branches     1497     1502       +5     
==========================================
+ Hits         6807     6816       +9     
- Misses        671      672       +1     

Files with missing lines	Coverage Δ
...de/opentelemetry-instrumentation-knex/src/utils.ts	91.48% <100.00%> (+1.24%)	⬆️
...emetry-instrumentation-knex/src/instrumentation.ts	97.67% <87.50%> (-1.11%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[scottgreenup]

I'm interested in this pull request getting merged, so I thought I'd add some notes.

It looks like connectionString is for both PostgreSQL and RedShift. It looks like the default port for RedShift is 5439.

I think you can check the URL protocol which should be postgres: or redshift: and set it to 5432/5439?

> (new URL("postgres://user:password@myhost/mydb")).protocol
'postgres:'

[edosrecki]

Thanks for the comment. Yes I can do that. However we cannot then cover the case of CockroachDB which uses postgres protocol, but has a different default port of 26257.

I will see how knex handles redshift connection string when port is not provided and try to match that functionality. In the end that is the right thing to do.

[pichlermarc]

It looks like there's another in-flight PR that will also affect the same part of the code, we'll wait for this to land first (#2671).

[deejay1]

Maybe we could pass the entire config.connection here and return the resulting values instead of having undefined checks in the attribute assignment further down?

[trentm]

There is also #2286
I haven't looked closely yet, but it is patching for a similar thing.
@deejay1 and others that use Knex: preferences for other PR?

[deejay1]

I think this settles having a custom function returning the connection attributes, when we now have three different ways to configure them, what do you think @edosrecki ?

I think it would be nice rolling this into this PR, I'd wait for @edosrecki to respond, if I'll make both changes it into a separate PR.

[maryliag]

Hi @edosrecki , thank you for your contribution. Recently the Database Semantic Conventions was marked as stabled, and with this a few changes came, for example, there are no longer attributes for Connection String, DB User, DB name, etc. And new attributes were added, such as DB namespace (that should include the removed attributes).
Here you can find a details doc about migrating to the stable convention.

Because of those changes, we don't want to add or update attributes that are no longer stable and should be removed instead. For this reason, I'll close this PR.

You can still open a new PR focusing only on fixes related to the stable convention, and you can also help out with the migration (which would be very much appreciate it and I would be happy to review), but I can see that a lot has already been done here, so you can check if any attributes are missing. Here is an example of how I'm doing this migration for the PG package.

Feel free to reach out to me if you have any questions!