Add abilty to hide db.statement in opentelemetry_redix
#189
Conversation
|
This is defined on documentation as a recommended attribute. Do you have examples of other libraries (java, node, ruby etc) that could optionally remove it? Also, is this causing issues? For example, Ecto statements tends to be even bigger than (most) Redis, still there is no configuration to opt-out this attribute. Sorry to push back a bit like that, just want to understand more the use case and keep as consistent as possible with prior art from OTEL community. Thanks in advance @tomtaylor! |
|
The docs actually say it should only be collected if the statement is sanitized. I forget where we are at with Ecto but it should not be collected by default because we do not sanitize. |
|
Ah right, its in, #166 |
|
I’m away at the moment without my laptop, but a cursory look showed me that the Ecto instrumentation supports a custom sanitiser as mentioned in #166. The Ruby Redis library supports something similar. Our example is that we do a Happy to turn this PR into a custom sanitiser if that’s what’s commonly implemented across multiple languages and there’s prior art in Elixir. Does that seem like a better approach? |
|
@tomtaylor yup, that'd be great |
|
@tomtaylor @tsloughter just a heads up, I'm on finishing points with a mongo instrumentation and following that same pattern, with a :disabled, :plain, :obfuscated (basic sanitization) and custom fun options. Sounds like a good plan to me! |
|
@tomtaylor any update on the switch to a function from a boolean? |
|
I haven't had time to dig into this yet. Happy for someone else to chip it in, or I'll try and get to it when I've got a bit of time. |
It's not always necessary/desired to include this attribute, especially as in some cases it can be very long.
This has introduced a breaking change, by moving from a boolean config to `:enabled`, `:disabled`, `function/1` as the config options for `db_statement`.
tomtaylor
force-pushed
the
redix-ability-to-hide-db-statement
branch
from
f6b7993 to
bd2ec4d
Compare
|
@tsloughter I've just sorted this and moved from a boolean config option to |
|
|
||
| You may also supply the following options: | ||
|
|
||
| * `db_statement` - `:enabled`, `:disabled` or a single arity function. If |
There was a problem hiding this comment.
Nit:
| * `db_statement` - `:enabled`, `:disabled` or a single arity function. If | |
| * `:db_statement` - `:enabled`, `:disabled` or a single arity function. If |
| @@ -27,19 +27,30 @@ defmodule OpentelemetryRedix do | |||
|
|
|||
| @doc """ | |||
| Initializes and configures the telemetry handlers. | |||
|
|
|||
| You may also supply the following options: | |||
There was a problem hiding this comment.
My suggestion: I would either go with :sanitized_and_logged | :not_logged | fun, or just true | false | fun.
| * `db_statement` - `:enabled`, `:disabled` or a single arity function. If | ||
| `:enabled`, the DB statement is sanitized of sensitive values, such as | ||
| AUTH crendentials, but otherwise logged. If `:disabled`, the statement is | ||
| not logged. If you pass in a single arity function, the list of commands |
There was a problem hiding this comment.
My suggestion is to have a type for the function to point to and document better. For example:
@type db_statement_fun() :: (Redix.command() -> String.t())
It's not always necessary/desired to include this attribute, especially as in some cases it can be very long.