Skip to content

Add Filter option to StackExchangeRedisInstrumentationOptions #2804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Jun 6, 2025

Conversation

aradalvand
Copy link
Contributor

@aradalvand aradalvand commented May 22, 2025

Fixes #2803

Changes

  • Added a Filter option (in line with conventions that most libraries in this repo follow) to StackExchangeRedisInstrumentationOptions.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

Sorry, something went wrong.

Copy link

linux-foundation-easycla bot commented May 22, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot requested a review from matt-hensley May 22, 2025 15:09
@github-actions github-actions bot added the comp:instrumentation.stackexchangeredis Things related to OpenTelemetry.Instrumentation.StackExchangeRedis label May 22, 2025
@aradalvand aradalvand marked this pull request as ready for review May 22, 2025 15:18
@aradalvand aradalvand requested a review from a team as a code owner May 22, 2025 15:18
@github-actions github-actions bot added the infra Infra work - CI/CD, code coverage, linters label May 22, 2025
@aradalvand aradalvand requested a review from matt-hensley May 23, 2025 00:03
@aradalvand
Copy link
Contributor Author

aradalvand commented May 23, 2025

Hi @matt-hensley :)
Could you please review this again, when you have the time? I went ahead an applied your suggestions.

aradalvand and others added 3 commits May 24, 2025 16:47
…angeRedisInstrumentationOptions.cs

Co-authored-by: Martin Costello <martin@martincostello.com>
…angeRedisInstrumentationOptions.cs

Co-authored-by: Martin Costello <martin@martincostello.com>
@github-actions github-actions bot removed the infra Infra work - CI/CD, code coverage, linters label May 24, 2025
@aradalvand aradalvand requested a review from martincostello May 25, 2025 01:29
@aradalvand
Copy link
Contributor Author

Could this be merged now that it's been approved?

@rajkumar-rangaraj rajkumar-rangaraj requested a review from Copilot May 29, 2025 22:38
Copilot

This comment was marked as outdated.

@rajkumar-rangaraj
Copy link
Contributor

@aradalvand tests are failing could you please check.

@aradalvand
Copy link
Contributor Author

aradalvand commented May 30, 2025

@rajkumar-rangaraj The failing tests all seem to be connection failures:

image

Can you please tell me what I should do? I'm confused.

@rajkumar-rangaraj
Copy link
Contributor

@rajkumar-rangaraj The failing tests all seem to be connection failures:

image

Can you please tell me what I should do? I'm confused.

@matt-hensley Do you any idea why tests fail here?

Copy link
Contributor

@matt-hensley matt-hensley left a comment

Choose a reason for hiding this comment

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

FilterOption_FiltersOutSpecifiedCommands issues commands, and even if all commands are filtered out the driver still tries to connect to an instance, making it more of an integration test. To work as expected it needs the [SkipUnlessEnvVarFoundTheory(RedisEndPointEnvVarName)] attribute added and then the connection setup modified to respect that env var.

A filter specific test can be added to RedisProfilerEntryToActivityConverterTests that does not require a connection:

    [Fact]
    public void ProfilerCommandToActivity_UsesFilter()
    {
        var activity = new Activity("redis-profiler");
        var profiledCommand = new TestProfiledCommand(DateTime.UtcNow);
        var options = new StackExchangeRedisInstrumentationOptions
        {
            Filter = command => false,
        };
        var result = RedisProfilerEntryToActivityConverter.ProfilerCommandToActivity(activity, profiledCommand, options);
        Assert.Null(result);
    }
    ```

aradalvand and others added 2 commits June 4, 2025 16:02
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds a Filter option to StackExchangeRedisInstrumentationOptions enabling early filtering of profiled commands.

  • Introduce a Filter property on the options class
  • Apply the filter in ProfilerCommandToActivity to skip unwanted commands
  • Update tests, changelog, and public API to include the new option

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/OpenTelemetry.Instrumentation.StackExchangeRedis/StackExchangeRedisInstrumentationOptions.cs Add Filter property and XML documentation
src/OpenTelemetry.Instrumentation.StackExchangeRedis/Implementation/RedisProfilerEntryToActivityConverter.cs Invoke Filter callback and skip commands when it returns false or throws
test/OpenTelemetry.Instrumentation.StackExchangeRedis.Tests/StackExchangeRedisCallsInstrumentationTests.cs Add a test validating that GET commands are filtered out
src/OpenTelemetry.Instrumentation.StackExchangeRedis/CHANGELOG.md Document the new Filter option in unreleased section
src/OpenTelemetry.Instrumentation.StackExchangeRedis/.publicApi/PublicAPI.Unshipped.txt Expose the Filter getter and setter in the public API
Comments suppressed due to low confidence (2)

src/OpenTelemetry.Instrumentation.StackExchangeRedis/StackExchangeRedisInstrumentationOptions.cs:25

  • The XML documentation for Filter is misleading; clarify that the predicate returns true to include (collect) events and false (or exceptions) to filter them out.
/// <summary>

test/OpenTelemetry.Instrumentation.StackExchangeRedis.Tests/StackExchangeRedisCallsInstrumentationTests.cs:389

  • Add a test case to verify behavior when the filter callback throws an exception to ensure it correctly filters out the command.
public void FilterOption_FiltersOutSpecifiedCommands(string value)

@aradalvand
Copy link
Contributor Author

@rajkumar-rangaraj Could you please consider merging? Thanks.

@rajkumar-rangaraj
Copy link
Contributor

Thanks @aradalvand for your contribution!

@rajkumar-rangaraj rajkumar-rangaraj merged commit a68ed98 into open-telemetry:main Jun 6, 2025
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.stackexchangeredis Things related to OpenTelemetry.Instrumentation.StackExchangeRedis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature request] Add Filter to StackExchangeRedisInstrumentationOptions
4 participants