Skip to content
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

[Instrumentation.StackExchangeRedis] Support when IConnectionMultiplexer is added with keyed service #1885

Merged
merged 15 commits into from
Jun 29, 2024

Conversation

Kahbazi
Copy link
Contributor

@Kahbazi Kahbazi commented Jun 13, 2024

Fixes #1451.

Changes

Added a new overload to AddRedisInstrumentation which gets serviceKey in input and get IConnectionMultiplexer with a keyed service.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@Kahbazi Kahbazi requested a review from a team June 13, 2024 08:36
@github-actions github-actions bot added the comp:instrumentation.stackexchangeredis Things related to OpenTelemetry.Instrumentation.StackExchangeRedis label Jun 13, 2024
@Kielek Kielek changed the title Kahbazi/redis keyed service [Instrumentation.StackExchangeRedis] Support when IConnectionMultiplexer is added with keyed service Jun 13, 2024
@Kielek
Copy link
Contributor

Kielek commented Jun 13, 2024

@eerhardt, please check this PR, you were reviewing the first version.

@Kielek Kielek requested a review from CodeBlanch June 14, 2024 04:47
Copy link
Contributor

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Looks good to me. Could probably use a few new tests.

Comment on lines +52 to +69
public static TracerProviderBuilder AddRedisInstrumentation(
this TracerProviderBuilder builder,
object serviceKey)
{
Guard.ThrowIfNull(serviceKey);

return AddRedisInstrumentation(builder, name: null, connection: null, serviceKey, configure: null);
}

/// <summary>
/// Enables automatic data collection of outgoing requests to Redis.
/// </summary>
/// <param name="builder"><see cref="TracerProviderBuilder"/> being configured.</param>
/// <param name="name">Optional name which is used when retrieving options.</param>
/// <param name="serviceKey">Optional service key used to retrieve the <see cref="IConnectionMultiplexer"/> to instrument from the <see cref="IServiceProvider" />.</param>
/// <param name="configure">Callback to configure options.</param>
/// <returns>The instance of <see cref="TracerProviderBuilder"/> to chain the calls.</returns>
public static TracerProviderBuilder AddRedisInstrumentation(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these new overloads have tests covering them?

Copy link

codecov bot commented Jun 24, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 69.92%. Comparing base (71655ce) to head (9ab8dae).
Report is 348 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1885      +/-   ##
==========================================
- Coverage   73.91%   69.92%   -3.99%     
==========================================
  Files         267        5     -262     
  Lines        9615      276    -9339     
==========================================
- Hits         7107      193    -6914     
+ Misses       2508       83    -2425     
Flag Coverage Δ
unittests-Instrumentation.StackExchangeRedis 69.92% <50.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...ckExchangeRedis/TracerProviderBuilderExtensions.cs 83.33% <50.00%> (-8.16%) ⬇️

... and 266 files with indirect coverage changes

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM

@Kielek Kielek closed this Jun 28, 2024
@Kielek Kielek reopened this Jun 28, 2024
@Kielek Kielek closed this Jun 29, 2024
@Kielek Kielek reopened this Jun 29, 2024
@Kielek Kielek merged commit 4c69afa into open-telemetry:main Jun 29, 2024
70 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.

Support Redis instrumentation when IConnectionMultiplexer is added with keyed service
4 participants