Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

Redis: don't record values that are written to the DB #50

Merged
merged 1 commit into from
Sep 29, 2020

Conversation

owais
Copy link
Contributor

@owais owais commented Sep 29, 2020

Redis instrumentation now skips recording actual values written to the
database for known write commands.

Redis instrumentation now skips recording actual values written to the
database for known write commands.
@owais owais requested review from pjanotti, johnbley and a team September 29, 2020 00:56
argsLength := len(parts) - 1
numArgs, ok := cmdCaptureArguments[cmd]
if !ok {
return cmdStr, cmd, argsLength
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this logic correctly, any unrecognized/not-explicitly-encoded command will just have part[0] (the command name and only the command name, no args) as the span name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the reverse. Any unrecognized command will use the full command with args as the tag value (preserve current behavior).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps for safety it should be the other way? The Jedis (Java) instrumentation appears to just use the command name (first part) and nothing else (though I like the idea of porting the "this number of params is safe" there).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The list of known commands is quite exhaustive. I actually went over all commands and listed down every command I found that wrote data to the DB or queried data using values that might be in the DB. All other commands either use keys/IDs and never take any "data" as arguments so it's safe to keep them at least from data volume perspective. The downside is that a new "write" command in future would escape this filter but it's probably not that big of a deal given than this PR mainly tries to reduce trace volume.

@owais
Copy link
Contributor Author

owais commented Sep 29, 2020

Merging this but please feel free to review and provide any addition comments @johnbley @pjanotti. I'll fix in a follow up PR.

@owais owais merged commit 174c4a8 into master Sep 29, 2020
@owais owais deleted the redis-db-cmd-tag branch September 29, 2020 18:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants