Skip to content

Conversation

@senocak
Copy link

@senocak senocak commented Oct 8, 2025

Since redis-om uses Jedis behind the scenes, there are 2 open pull requests on Jedis repository for a long time to implement listener, I do not think it will be merged very soon. So, implemeted basic listener in each redisearch functionallity to listen the operations

Using in client side is very staight forward;

@Component
public class LoggingCommandListener implements CommandListener {
    private static final Logger log = LoggerFactory.getLogger(LoggingUnifiedJedis.class);

    @Override
    public void searchStarted(String indexName, Query q, FTSearchParams params) {
        try {
            final Field field = q.getClass().getDeclaredField("_queryString");
            field.setAccessible(true);
            String queryString = (String) field.get(q);
            log.info("Command Started: FT.SEARCH '{}' '{}'", indexName, queryString);
        } catch (NoSuchFieldException | IllegalAccessException e) {
            log.error("Command started on index: {} (unable to retrieve query string)", indexName);
        }
    }

    @Override
    public void searchFinished(String indexName, Query q, FTSearchParams params, SearchResult searchResult) {
        try {
            final Field field = q.getClass().getDeclaredField("_queryString");
            field.setAccessible(true);
            String queryString = (String) field.get(q);
            log.info("Command Finished: FT.SEARCH '{}' '{}' - Total result: {} ", indexName, queryString, searchResult.getTotalResults());
        } catch (NoSuchFieldException | IllegalAccessException e) {
            log.error("Command started on index: {} (unable to retrieve query string)", indexName);
        }
    }
}

@jit-ci
Copy link

jit-ci bot commented Oct 8, 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.

@senocak
Copy link
Author

senocak commented Oct 15, 2025

@foogaro Kindly reminder please!

@bsbodden
Copy link
Collaborator

@foogaro Kindly reminder please!

Thanks for the PR, we will likely added to the Spring 4.0.0 upgrade release

@bsbodden bsbodden self-assigned this Nov 26, 2025
@bsbodden
Copy link
Collaborator

Hi @senocak, thank you for this contribution! The CommandListener feature is a great addition for observability and debugging Redis operations.

I've reviewed the PR and have a few items that need to be addressed before we can merge:

1. Rebase Required

The PR is based on an older version of main and conflicts with recent changes. When rebased, it should pick up:

Please rebase onto the latest main branch.

2. Remove @Component from NoOpCommandListener

The NoOpCommandListener class has both @Component annotation and is created via @Bean @Fallback in the configuration. This creates duplicate bean definitions. Please remove the @Component annotation since the @Bean @Fallback approach is the correct pattern here.

// Remove @Component here
public class NoOpCommandListener implements CommandListener {
}

3. Add Tests for Listener Functionality

The PR adds the feature but doesn't include tests to verify the listener callbacks are actually invoked. Please add a test that:

  • Creates a test listener implementation that tracks invocations
  • Performs a search operation
  • Verifies searchStarted and searchFinished were called with correct parameters

4. Consider Exception Handling (Optional)

Consider wrapping listener invocations in try-catch so that a failing listener doesn't break the actual Redis operation. This would make the feature more robust in production environments.


Once these items are addressed, we'll be happy to merge this. Thanks again for the contribution!

…eFinished and createIndex methods and write more tests to cover the listeners. Use try-catch-finally method for exception handling
…d-listener' into feature/senocak/add-redis-command-listener

# Conflicts:
#	redis-om-spring/src/main/java/com/redis/om/spring/ops/CommandListener.java
#	redis-om-spring/src/main/java/com/redis/om/spring/ops/NoOpCommandListener.java
#	redis-om-spring/src/main/java/com/redis/om/spring/ops/search/SearchOperationsImpl.java
@senocak
Copy link
Author

senocak commented Nov 27, 2025

Hi @senocak, thank you for this contribution! The CommandListener feature is a great addition for observability and debugging Redis operations.

I've reviewed the PR and have a few items that need to be addressed before we can merge:

1. Rebase Required

The PR is based on an older version of main and conflicts with recent changes. When rebased, it should pick up:

Please rebase onto the latest main branch.

2. Remove @Component from NoOpCommandListener

The NoOpCommandListener class has both @Component annotation and is created via @Bean @Fallback in the configuration. This creates duplicate bean definitions. Please remove the @Component annotation since the @Bean @Fallback approach is the correct pattern here.

// Remove @Component here
public class NoOpCommandListener implements CommandListener {
}

3. Add Tests for Listener Functionality

The PR adds the feature but doesn't include tests to verify the listener callbacks are actually invoked. Please add a test that:

  • Creates a test listener implementation that tracks invocations
  • Performs a search operation
  • Verifies searchStarted and searchFinished were called with correct parameters

4. Consider Exception Handling (Optional)

Consider wrapping listener invocations in try-catch so that a failing listener doesn't break the actual Redis operation. This would make the feature more robust in production environments.

Once these items are addressed, we'll be happy to merge this. Thanks again for the contribution!

Thank you @bsbodden I did every item you highlighted. If there is anything else, i surely can make them done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants