Skip query sanitization for prepared statements under stable semconv flag#15855
Conversation
5c413eb to
52c940f
Compare
09a4532 to
aca641d
Compare
| Long batchSize = getter.getBatchSize(request); | ||
| boolean isBatch = batchSize != null && batchSize > 1; | ||
| boolean parameterizedQuery = getter.isParameterizedQuery(request); | ||
| boolean shouldSanitize = statementSanitizationEnabled && !parameterizedQuery; |
There was a problem hiding this comment.
Can we do this right now or should this be done along with switching to the stable semconv? Maybe
statementSanitizationEnabled && (!SemconvStability.emitStableDatabaseSemconv() || !parameterizedQuery)? Or maybe we should introduce a flag for future defaults so we could implement the planned changes for 3.0 straight away but have them disabled for now.
There was a problem hiding this comment.
oh, it's only used in the experimental semconv block, I moved it down inside that block make it more clear now
1885a18 to
dbdf02d
Compare
fa17edf to
1e97c5b
Compare
PreparedStatement queries use placeholders and are already parameterized, so they don't need sanitization. This change adds an isPreparedStatement flag to DbRequest and SqlClientAttributesGetter to skip sanitization for prepared statements while still sanitizing regular Statement queries.
1e97c5b to
b6bcd8b
Compare
| * of db.query.text</a>. | ||
| */ | ||
| // TODO: make this required to implement | ||
| default boolean isParameterizedQuery(REQUEST request) { |
There was a problem hiding this comment.
wondering whether it would be better to name this method based on what it is used for e.g. IsQuerySanitizationNeeded
There was a problem hiding this comment.
I think it makes sense, I'll send a follow-up PR to consider
https://github.com/open-telemetry/semantic-conventions/blob/main/docs/db/database-spans.md#sanitization-of-dbquerytext
TODO add opt-in to sanitize anyways?