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

fix(knex): connection attrs missing if connectionString used #1932

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

edosrecki
Copy link

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:
Screenshot 2024-02-10 at 14 28 28

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:

Screenshot 2024-02-10 at 15 43 55

Copy link

codecov bot commented Feb 12, 2024

Codecov Report

Merging #1932 (d6e6090) into main (7895306) will decrease coverage by 0.01%.
Report is 1 commits behind head on main.
The diff coverage is 92.85%.

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 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%) ⬇️

const url = new URL(connectionString);
return {
host: url.hostname,
port: parseInt(url.port || '5432', 10),

Choose a reason for hiding this comment

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

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:'

Copy link
Author

@edosrecki edosrecki Mar 27, 2024

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants