-
Notifications
You must be signed in to change notification settings - Fork 179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: create span for mysql2 execute #1221
base: main
Are you sure you want to change the base?
feat: create span for mysql2 execute #1221
Conversation
|
||
def _otel_execute_attributes(args, kwargs) | ||
if config[:db_statement] == :include | ||
{'args' => args.to_s, 'kwargs' => kwargs.to_s} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree that we should amend all attributes without using explicit mappings for semantic conventions especially if it opens up a risk to exposing sensitive data.
E.g. auth token or password would be exposed through telemetry collection, with a safeguards to remove them.
I do realize we allow this to happen today with our existing include
statement but I am leaning more and more each day to deleting that option all together and only allowing sanitized
, obfuscate
, or omit
in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does sanitized
mean specifically removing the sensitive data (e.g. token, password, ssn, health_id, etc.), but not other binding parameters such as address?
I believe most password stored in table should be hashed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does sanitized mean specifically removing the sensitive data (e.g. token, password, ssn, health_id, etc.), but not other binding parameters such as address?
I think our current instrumentation uses the wrong vocabulary to describe what it does. Our current instrumentations use the word obfuscate
, when I think we really mean is to sanitize
, i.e. remove all user provided values.
I do not mean only targeting specific sensitive attributes when I use the word sanitize
.
I now think of obfuscation
as meaning that the entire query is hashed as opposed to just the parameters or values.
I brought up a request to change our vocabulary at a SIG meeting and the result #1194
Related to this topic, there was a recent OTEP submitted to around sensitive data redaction but I have not taken the time to digest it all just yet:
There are now experimental attributes db.operation.parameter.<key> https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/sql.md#attributes |
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
Description
Adding a span for the mysql2 execute function is similar to the pg
EXEC_PREPARED_ISH_METHODS
.When using prepared statements with ActiveRecord, the first execution of a query will create a prepare span, which can log the query execution. However, re-execution will not trigger the prepare function; it will trigger the execute function directly. Since we currently don’t create a span for the execute function, I’d like to propose adding a span for the execute function.
See prepared statement execution log from mysql server:
Even though there is no valuable information except the binding value for the prepared statement, it is useful for monitoring the MySQL query execution duration.
Sample span data: