Add WithDBStatement to add db.statement attribute to rueidisotel

[chkp-omris]

Add tracer option to add the whole command's statement to db.statement attribute.

Need this for integration with APM and couldn't find another way that doesn't involve copying a lot of code from otelClient's implementation.

I mostly use non-multi operations and they seem a bit more complex and opinionated so I didn't implement it on my own.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 56.09756% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 95.53%. Comparing base (a74b679) to head (f399414).
Report is 5 commits behind head on main.

Files	Patch %	Lines
rueidisotel/trace.go	56.09%	12 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #523      +/-   ##
==========================================
- Coverage   95.57%   95.53%   -0.05%     
==========================================
  Files          80       80              
  Lines       33178    33204      +26     
==========================================
+ Hits        31710    31721      +11     
- Misses       1267     1278      +11     
- Partials      201      205       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rueian]

Hi @chkp-omris,

Unlike other database systems, capturing the full redis command into db.statement is probably a terrible idea. The full command could not only be huge but also contain credentials that shouldn’t be leaked to APM.

Could you just capture at most two tokens from a command? For example, the db.statement of SET mykey mysecret command could be just SET mykey.

[chkp-omris]

@rueian thanks for the quick response.
I agree it's not a great idea, I saw that go-redis implements this so thought rueidis can have that too.
I looked at go-redis's implementation again, I missed that it does truncate/limit the statement.
go-redis puts the result of CmdString in the db.statement attribute, which limits the string's size to 32 bytes.

I think it's reasonable to have the same limitation in rueidis's implementation, what do you think?

[rueian]

32 bytes capping is good, but I think we should also take only the first two tokens to avoid capturing any credentials.

[chkp-omris]

@rueian First of all I hope you and your loved ones are safe after the earthquake in Taiwan.

32 bytes capping is good, but I think we should also take only the first two tokens to avoid capturing any credentials.

It depends on the user's use case.
In my case it isn't an issue, and of course for some it is an issue, but that's why this option is off by default.
I think it would be better to add this functionality and expand it for other use cases when requested.

Also, I suggest applying the 32 bytes cap to each token in the command, as there could be flags/options given that are important.
For example, when using FT.SEARCH the options given to it can change its meaning quite a lot.
We can cap the number of tokens it will take of the command as well to like 10.

Let me know what you think and I'll implement it.

[rueian]

First of all I hope you and your loved ones are safe after the earthquake in Taiwan.

Thank you! We are fine.

I think it would be better to add this functionality and expand it for other use cases when requested.

So, instead of WithDBStatement(enabled bool), how about WithDBStatement(stmtFn func(command []string) (stmt string))?

In this way, users have full responsibility for the statement being sent.

[chkp-omris]

So, instead of WithDBStatement(enabled bool), how about WithDBStatement(stmtFn func(command []string) (stmt string))?

In this way, users have full responsibility for the statement being sent.

Yea that's a good idea, done

[rueian]

Hi @chkp-omris, LGTM! But these SetAttributes are not covered by tests

Would you mind to add tests for them?

[chkp-omris]

@rueian Sure, I added the option to the unit tests for the client

[rueian]

👍