Skip to content

Conversation

@Udhayarajan
Copy link
Contributor

@Udhayarajan Udhayarajan commented Oct 20, 2025

This PR is to address #3479.

While #3519 solves the issue partially (for Process Hook).

In this, I've added similar filters for dial hook and process pipeline hook

@Udhayarajan Udhayarajan changed the title (feat): add trace filter for process pipeline and dial operation feat(tracing): add trace filter for process pipeline and dial operation Oct 20, 2025
@jit-ci
Copy link

jit-ci bot commented Oct 20, 2025

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.

@Udhayarajan Udhayarajan marked this pull request as ready for review October 20, 2025 13:03
@Udhayarajan
Copy link
Contributor Author

Looks like the CI failed due to Docker pull issues on test images, not related to the code changes.
Can someone please re-run the pipeline? Or should I push empty commit to retrigger it?

@ndyakov
Copy link
Member

ndyakov commented Oct 21, 2025

Hello @Udhayarajan, I just reran the pipeline for you.

@ndyakov ndyakov self-requested a review October 21, 2025 09:17
@ndyakov
Copy link
Member

ndyakov commented Oct 21, 2025

@Udhayarajan what do you think about having the BasicCommandFilter renamed to DefaultCommandFilter and applied for all possible hooks, so we are sure by default the auth command is filtered as well as hello when there is a password in it?

@Udhayarajan
Copy link
Contributor Author

@Udhayarajan what do you think about having the BasicCommandFilter renamed to DefaultCommandFilter and applied for all possible hooks, so we are sure by default the auth command is filtered as well as hello when there is a password in it?

that's really sounds good, I'll make the changes

@Udhayarajan
Copy link
Contributor Author

@ndyakov Pushed the changes, taking reference from one of the older PRs.
Keeping DefaultCommandFilter exported so users can chain them with custom ones if needed.

I also saw @htemelski-redis had a different view earlier #3519 (comment)

@ndyakov
Copy link
Member

ndyakov commented Oct 22, 2025

Yes, but it make sense to have it as default. Just because we had already released a version, keep the old name as well, so we don't break the contract.

@Udhayarajan
Copy link
Contributor Author

DefaultCommandFilter definitely sounds more appropriate than BasicCommandFilter. Since a version with the old name is already released, I’d suggest keeping both for now, make BasicCommandFilter just a wrapper around DefaultCommandFilter and mark it as deprecated for backward compatibility.

Copy link
Member

@ndyakov ndyakov left a comment

Choose a reason for hiding this comment

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

Thank you @Udhayarajan

@ndyakov ndyakov merged commit 70dfa38 into redis:master Oct 22, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants