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

pg instrumentation records wrong db connection string #782

Closed
davazp opened this issue Dec 9, 2021 · 5 comments
Closed

pg instrumentation records wrong db connection string #782

davazp opened this issue Dec 9, 2021 · 5 comments
Labels
bug Something isn't working stale

Comments

@davazp
Copy link

davazp commented Dec 9, 2021

What version of OpenTelemetry are you using?

"@opentelemetry/auto-instrumentations-node": 0.26.1

What version of Node are you using?

v16.13.0

What did you do?

The pg instrumentation records the connection string as localhost on pg-pool.connect even when that is not the right value. The problem is here:

export function getJDBCString(params: PgClientConnectionParams) {
const host = params.host || 'localhost'; // postgres defaults to localhost
const port = params.port || 5432; // postgres defaults to port 5432
const database = params.database || '';
return `jdbc:postgresql://${host}:${port}/${database}`;
}

The logic in pg is of course more complex than that. In particular, it looks at the PGHOST environment variable that I am setting.

So pg connects to the right host, but it is recorded in the span as localhost.

This bug is not present in the .query spans because we look at the parameters of the pg client. Unfortunately, pg-pool doesn't know about how the parameters. They aren't resolved yet.

What did you expect to see?

pg-pool.connect should not show the wrong connection string.

What did you see instead?

localhost

Additional context

I don't think that replicating the same logic is sensitive here. Among other things, reading environment variables might have performance implications.

One conservative option is to not include the connection string if we don't have all the values, instead of defaulting to localhost (and same for the port 5432).

Other ideas? I can try to help with a PR.

@davazp davazp added the bug Something isn't working label Dec 9, 2021
@davazp davazp changed the title pg instrumentation records wrong connection string pg instrumentation records wrong db connection string Dec 9, 2021
@davazp
Copy link
Author

davazp commented Dec 9, 2021

It's easy to workaround this by defining host like host: process.env.PGHOST when defining the pg pool.

@dyladan
Copy link
Member

dyladan commented Dec 9, 2021

This will likely be resolved by #781 when we change to use the connection string instead of a built jdbc string yes?

@davazp
Copy link
Author

davazp commented Dec 9, 2021

Will it? I can keep an eye on it. But I don't know how the span at pg-pool.connect will know the target of the client.

@github-actions
Copy link
Contributor

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Feb 14, 2022
@github-actions
Copy link
Contributor

This issue was closed because it has been stale for 14 days with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stale
Projects
None yet
Development

No branches or pull requests

2 participants