Avoid attaching raw query to spans

[OriolMunoz]

I found #653

This is relevant for us too.

From what I can see in

Kamon/instrumentation/kamon-jdbc/src/main/scala/kamon/instrumentation/jdbc/StatementMonitor.scala

Line 86 in 2235b79

val clientSpan = Kamon.clientSpanBuilder(spanName, "jdbc").tag("db.statement", sql)

, it does not appear that there's a way to do so currently, as the db.statement is unconditionally added as tag. Am I correct? Is there another way?

SpanCustomizer or Obfuscation are proposed in the issue, but it doesn't seem like they exist (anymore?).

[ivantopo]

The SpanCustomizer thing is gone and we have pre-start/pre-finish hooks now. Still, I think that this should be solved at a different level. The very first idea that comes to mind is to have a simple configuration where we could decide what tags should be sanitized and what regexes should be used for that. For example, something like this:

kamon.trace.data-protection {
  all {
    email-address = [
      "...regex...",
      "...regex..."
    ]
  }

  db.statement = [
    user-id = [
      "...regex..."
  ]
}

The configuration above would put [email-address] in place of an email address in any tag that matches the email-address regexes, and put [user-id] wherever matches the patterns in the db.statement tag only. The cost of sanitizing these tags can be kind of high depending on how complex the regexes are, but in certain situations there is no way around it.

Does this sound like a doable solution? is there a better way to do this?

NOTE: the JDBC instrumentation should be taking the SQL from your prepared statements and seeing the actual pattern with question marks. Are you maybe not using prepared statements or could you please share a bit more details about the your libraries and versions? Maybe we also need to do some extra work in that direction.

[OriolMunoz]

Does this sound like a doable solution?

From a library standpoint this is probably the nicest solution :)
We would even be happy with just removing the db.statement altogether, but I can see how your proposed solution is more flexible.

the JDBC instrumentation should be taking the SQL from your prepared statements and seeing the actual pattern with question marks. Are you maybe not using prepared statements [...]

We use Slick for most of our queries. Where we have to use raw queries, we use String interpolation where it should use prepared statements afaik.
For the most part, it does create prepared statements, but there are instances where it doesn't. One such case is, e.g.:

protected class Users(tag: Tag) extends Table[Model](tag, "USER") { [...] }

object Users extends TableQuery(new Users(_)) {

  case class GlobalUserId(value: UUID)

  implicit val globalUserIdMapper =
    MappedColumnType.base[GlobalUserId, String](_.toString, stringId => GlobalUserId(parseUUID(stringId, GlobalUserId)))

  type RichQuery = Query[Users, Model, Seq]
  
  def globalUserId(globalUserId: Rep[GlobalUserId]): RichQuery = {
    this.filter(_.globalUserId === globalUserId)
  }
}

which is then used:

def findByGlobalUserId(globalUserId: GlobalUserId): Future[Option[User]] = {
    db.run {
      Users.globalUserId(globalUserId).result.headOption
    }
  }

yields:

select [many columns] from `USER` where `GLOBAL_USER_ID` = '3d5c5be0-5fb1-0000-0000-485a7b1528c5'

Maybe it's because of the globalUserIdMapper? I don't see another way of doing this, however.

could you please share a bit more details about the your libraries and versions?

We're using Slick 3.3.3 - let me know if you think another dependency would be the culprit.

[OriolMunoz]

With a simple insert, from:

val process = UserImportProcess([...])
    db.run {
      UserImportProcesses += process
    }

we get:

insert into `BULK_USER_IMPORT_PROCESS` (columns)  values (?,?,?,?,?,?)

[ivantopo]

Hey @OriolMunoz, I was digging a bit deeper into this and realized that we could safely add the db.statement tag when using prepared statements. I went ahead and implemented a new setting in the jdbc instrumentation for that. You can check it out in this PR: #1103

If nothing weird comes up we should be merging and releasing it by the end of this week.

[OriolMunoz]

This looks great, thank you very much!

[OriolMunoz]

@ivantopo I found out that this is a Slick issue: it always generates prepared statements, but just adds whatever parameter you use directly onto the query instead of as ? and binding it later.
This appears to only happen for selects, but not for inserts or updates.
Likewise, it only happens for non-compiled queries, but when using Compiled it uses ? as expected.
It's unclear to me right now if this is intentional or not - I'll keep investigating.