Added JDBC db.query.parameter span attributes

[AlixBa]

Contribution for #7413

Not entirely sure about/need help/to finish

• the way to opt-in to this
• the stringify method for the attribute value

What does it do when opted-in?

query: SELECT * FROM table WHERE id=3
-> db.statement = SELECT * FROM table WHERE id=3
-- disable sanitization

query: SELECT * FROM table WHERE id=?
statement.setParameter(1, 3::int)
-> db.statement = SELECT * FROM table WHERE id=?
-> db.operation.parameter.0=3

Stringified types:

• numbers
• strings
• dates

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: AlixBa / name: Alix (26d635c, 235f221, 32aeb83, 2d48543, b3365f1, f6702f0, a785cd6, 9d6f08a, 07f8c5f, bf67533, d1ebd7e, 99ce35f, 18eb6f8)

[AlixBa]

Why is the 3 sanitized in this case? (not related to this PR, just a regular question)

[laurit]

Sanitizer replaces all literals with ?. It does not distinguish where the literal appears or whether it just a random 3 or something sensitive like a password or a credit card number. The purpose of the sanitizer is to protect the users and apm vendors from accidentally sending sensitive information to the apm. Without having looked too much into your PR In my opinion it might be best if you first focus on prepared statements, if users wishes to see parameter values in sanitized queries they can disable the sanitizer.

[trask]

it might be best if you first focus on prepared statements, if users wishes to see parameter values in sanitized queries they can disable the sanitizer

👍

[AlixBa]

My concern with this PR is more to see parameters of the prepared statement itself. Currently, if I am doing things right, even with the sanitizer disabled, here is what I get from the library when I wrap with an OpenTelmetryDataSource in one of my app

SELECT pg_advisory_xact_lock(('x' || md5(?))::bit(64)::bigint)

where as my initial scala code (Scala + Doobie) looks like this

  private def acquireLockByIdsNel(ids: NonEmptyList[String]): Query0[Unit] = {
    val unique = ids.distinct.sorted
    val frg    = unique.map(id => fr"pg_advisory_xact_lock(('x' || md5($id))::bit(64)::bigint)")
    val all    = fragments.comma(frg)
    (fr"SELECT" ++ all).queryWithLabel[Unit]("SELECT locks")
  }

as this get translated into a PreparedStatement and having no clue about what is currently sent to the DB, I'd like db.(operation|query).parameter to be set. Thing is, ? is already used by the sanitizer. So in order to make things clear for the user, I have to(?) expose them as parameters.

Let me know if I misunderstood or going the wrong direction!

[laurit]

Thing is, ? is already used by the sanitizer. So in order to make things clear for the user, I have to(?) expose them as parameters.

I wouldn't be too concerned with this. Firstly users can disable the sanitizer, as far as I can tell currently you basically attempt to undo what the sanitizer does by capturing the value. Secondly I'd expect that most queries that use prepared statements don't contain literals that the sanitizer removes. In your place I would start with capturing the parameters for prepared statements.

[AlixBa]

Then I think that this PR does exactly what I want!
I can remove the db.query.parameter for sanitized parameters from the query if that's an issue. But other than that, the PR captures the parameters from prepared statements

[trask]

I'd expect that most queries that use prepared statements don't contain literals that the sanitizer removes

ah, I think we should remove sanitization of prepared statement text in upcoming stable database semconv

Even though parameterized query text can potentially have sensitive data, by using a parameterized query the user is giving a strong signal that any sensitive data will be passed as parameter values, and the benefit to observability of capturing the static part of the query text by default outweighs the risk.

this will also ensure the ? placeholders line up with the db.query.parameter list

I've added this to our tracking issue #12608

[trask]

@AlixBa check out very recent semconv change: open-telemetry/semantic-conventions#2093

[AlixBa]

hey @laurit
Would you have time to do a first review of this PR? 🙏🏿
Let me know if there is anything I need to do first

[laurit]

This table should be reformatted. The description of the flag needs to include a warning. I'll leave it to @trask to come up with the exact wording, probably something along the lines of WARNING: captured query parameters may contain sensitive information such as passwords, personally identifiable information or protected health info. Exposing such info may result in substantial fines and penalties or criminal liability. Consult your peers, superiors and a legal counsel before enabling this option.

[trask]

it's a good question, I've asked in the community repo

[AlixBa]

should be added to metadata.yaml