Lettuce 5.1 instrumentation should log normalised commands as db.statement #1405
Conversation
mateuszrzeszutek
requested review from
anuraaga,
iNikem,
jkwatson,
trask and
tylerbenson
as code owners
| import java.util.Map; | ||
|
|
||
| /** This class is responsible for masking potentially sensitive data in Redis commands. */ | ||
| public final class RedisCommandNormalizer { |
There was a problem hiding this comment.
This class can be reused by other Redis libraries' instrumentations; I'll move it to some common package (javaagent-api?) in next PR and call it whenever args are available. Currently lettuce 5.1 is the only Redis lib instrumentation that logs actual statement, not just the bare command.
...ain/java/io/opentelemetry/javaagent/instrumentation/lettuce/v5_1/RedisCommandNormalizer.java
Outdated
Show resolved
Hide resolved
...ain/java/io/opentelemetry/javaagent/instrumentation/lettuce/v5_1/RedisCommandNormalizer.java
Outdated
Show resolved
Hide resolved
...ain/java/io/opentelemetry/javaagent/instrumentation/lettuce/v5_1/RedisCommandNormalizer.java
Outdated
Show resolved
Hide resolved
.../main/java/io/opentelemetry/javaagent/instrumentation/lettuce/v5_1/OpenTelemetryTracing.java
Outdated
Show resolved
Hide resolved
.../main/java/io/opentelemetry/javaagent/instrumentation/lettuce/v5_1/OpenTelemetryTracing.java
Outdated
Show resolved
Hide resolved
.../main/java/io/opentelemetry/javaagent/instrumentation/lettuce/v5_1/OpenTelemetryTracing.java
Outdated
Show resolved
Hide resolved
...tation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/utils/NetPeerUtils.java
Show resolved
Hide resolved
...ain/java/io/opentelemetry/javaagent/instrumentation/lettuce/v5_1/RedisCommandNormalizer.java
Outdated
Show resolved
Hide resolved
...ain/java/io/opentelemetry/javaagent/instrumentation/lettuce/v5_1/RedisCommandNormalizer.java
Outdated
Show resolved
Hide resolved
...ain/java/io/opentelemetry/javaagent/instrumentation/lettuce/v5_1/RedisCommandNormalizer.java
Show resolved
Hide resolved
| "${SemanticAttributes.NET_PEER_PORT.key}" port | ||
| "${SemanticAttributes.DB_CONNECTION_STRING.key}" "redis://127.0.0.1:$port" | ||
| "${SemanticAttributes.DB_SYSTEM.key}" "redis" | ||
| "${SemanticAttributes.DB_STATEMENT.key}" "SET a ?" |
There was a problem hiding this comment.
The rendering is a great improvement, thanks! But the spec seems to include argument values for statements
I don't know if this is a good idea though - do you mind filing a spec issue to discuss the PII implications and adding scrubbing to the spec?
There was a problem hiding this comment.
Forgot to mention the improved rendering is very nice!
There was a problem hiding this comment.
I think this is in accordance with spec - see db.statement attribute spec:
[3]: The value may be sanitized to exclude sensitive information.
Still, I think that this could be described in a more clear and explicit way. I'll create a spec issue to add an example for masking/sanitization.
There was a problem hiding this comment.
Forgot to mention the improved rendering is very nice!
Thanks 😄
There was a problem hiding this comment.
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
...rc/main/java/io/opentelemetry/javaagent/instrumentation/lettuce/v5_1/LettuceArgSplitter.java
Show resolved
Hide resolved
Resolves #1386