feat(otel): add unified filtering API with O(1) command exclusion

[zadil]

Add comprehensive filtering capabilities with performance-optimized command exclusion and unified filtering API.

• WithExcludedCommands(): O(1) command exclusion using map lookup
• WithProcessFilter(): Custom filtering for individual commands
• WithProcessPipelineFilter(): Custom filtering for pipeline operations
• WithDialFilterFunc(): Custom filtering for dial operations
• Backward compatible with existing filtering APIs
• Comprehensive test coverage and updated examples

Fixes #3479

[jit-ci]

Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset.

In case there are security findings, they will be communicated to you as a comment inside the PR.

Hope you’ll enjoy using Jit.

Questions? Comments? Want to learn more? Get in touch with us.

[ndyakov]

@zadil thank you for your contribution, will review it next week.

[ndyakov]

pinging @Udhayarajan and @lucawol for comments here as they were active in the discussion.

[Udhayarajan]

I've a few things to mark on.

0. excludedCommands

The goal is to filter in O(1). Anyone who want to achieve O(1) is still possible via existing methods. They can create a custom stuct and still can achieve the same (as I mentioned in discussion). Afaik go inline methods, when I tested with same I proposed in discussion I can see the go compiler says both FilterPipeline and FilterCommand can inline. The O(1) map lookup logic is now as efficient as possible since the filtering methods are inlined.

This excludeCommands just offers a way so that people no need to create their own struct to achieve O(1).

Trade off:

IMO, We are trading off library code quality with pre-cooked fields in config struct. I love the way you reduced double maintenance with simple helper functions, everything looks like over-kill, at lease to me ,as func(cmds []redis.Cmder) bool and func(cmd redis.Cmder) bool can do what excludedCommands does.

1. unified* vs filter* (excluding unifiedDialFilter)

everything is exactly the same do we really need to duplicate this fields? Though the version is released these are non-exported filed and shouldn't affect any user, may be we just have one field and use them in 2 different option functions, what's your opinion?

2. unifiedDialFilter

I designed dial filter assuming, user won't filter the trace based on network and address thus kept is simple. Current proposed approach gives better flexibility to filter the dial with help of address and port.

Trade off:

I don't think there is any critical trade-off, but in rare case, say user configured to filter from their current address and port but later if redis server is changed then they suddenly starting seeing trace. Again, I'm not sure is this trade-off or can this be even labeled as careless update by user. If I'm that user for sure I won't be remembering this part where I configured to ignore dial from certain address and port. But it offers great flexibility to users for filtering out dial for specific server in cluster mode.

I'm in favor of this unifiedDialFilter

@ndyakov pls add your inputs too.

cc: @htemelski-redis

[Udhayarajan]

I would like to pull in one more person, since we're just adding toppings to the base baked by @Sovietaced

[ndyakov]

Here are my 2 cents:
Let's think about the main usecase of those filter:

• Developers would like to filter traces/logs related to AUTH commands
• Developers may want to filter traces including specific keys/ key prefixes.

Although not the most performant approach, the way to address both would be to search not only the command name, but the body of the command itself. We could exclude both HELLO and AUTH for the AUTH case, but what about the keys?

I haven't benchmarked this, so take my suggestion with a grain of salt:

• I would have two filters, exclude command and exclude keys
• • exclude command would be the "faster" approach
• • exclude keys is tricky to implement, so maybe even we would like to skip it? If we do a string match we can end up matching the value and not a key.

Both filter should be applicable to both pipelines and normal commands.

As for the dialer, I do not think we need a filter there in general, don't have a strong opinion if we should keep one, if you think it is going to be useful, sure, let's keep it.

The map lookup may limit the flexibility of the filter, but at the same time gives the users a single way to filter a command, which should be less prone to errors from unexpected or wrong use of the filters.

I would vote for the map lookup. even, would store both uppercase and lowercase cmd names in the map and skip the strings.ToUpper on each execution.