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

[redis] Modernize the StackExchangeRedis project #1183

Merged

Conversation

CodeBlanch
Copy link
Member

Changes

  • Turn on nullable, implicit usings, and code analysis.
  • Take dependency on OpenTelemetry.Api.ProviderBuilderExtensions and update to the latest DI patterns (adds named options support and registers the configuration delegate (if supplied) into Options API).

TODOs

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

@CodeBlanch CodeBlanch requested a review from a team May 10, 2023 21:54
@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

Merging #1183 (2556026) into main (006c06e) will decrease coverage by 0.07%.
The diff coverage is 70.27%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1183      +/-   ##
==========================================
- Coverage   72.88%   72.82%   -0.07%     
==========================================
  Files         247      247              
  Lines        8852     8836      -16     
==========================================
- Hits         6452     6435      -17     
- Misses       2400     2401       +1     
Impacted Files Coverage Δ
...esourceDetectors.AWS/Models/AWSEBSMetadataModel.cs 100.00% <ø> (ø)
...etectors.AWS/Models/AWSEC2IdentityDocumentModel.cs 100.00% <ø> (ø)
...urceDetectors.AWS/Models/AWSEKSClusterDataModel.cs 100.00% <ø> (ø)
...ectors.AWS/Models/AWSEKSClusterInformationModel.cs 100.00% <ø> (ø)
src/Shared/ActivityInstrumentationHelper.cs 100.00% <ø> (ø)
src/Shared/DiagnosticSourceListener.cs 70.00% <ø> (ø)
src/Shared/DiagnosticSourceSubscriber.cs 94.73% <ø> (ø)
src/Shared/InstrumentationEventSource.cs 12.50% <ø> (ø)
src/Shared/MultiTypePropertyFetcher.cs 80.00% <ø> (ø)
src/Shared/PropertyFetcher.cs 75.00% <ø> (ø)
... and 13 more

... and 1 file with indirect coverage changes

@utpilla utpilla added the comp:instrumentation.stackexchangeredis Things related to OpenTelemetry.Instrumentation.StackExchangeRedis label May 10, 2023
@Kielek Kielek merged commit ae4e17d into open-telemetry:main May 11, 2023
@CodeBlanch CodeBlanch deleted the stackexchangeredis-registrationupdates branch May 11, 2023 16:42
/// </remarks>
/// <param name="builder"><see cref="TracerProviderBuilder"/> being configured.</param>
/// <returns>The instance of <see cref="TracerProviderBuilder"/> to chain the calls.</returns>
public static TracerProviderBuilder AddRedisInstrumentation(
Copy link

@niemyjski niemyjski Jul 5, 2023

Choose a reason for hiding this comment

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

These overloads are a breaking change. An update from 9.8 to latest caused a more specific overload to be called which threw an argument null exception:

b.AddRedisInstrumentation(null, c => {
                                c.EnrichActivityWithTimingEvents = false;
                                c.SetVerboseDatabaseStatements = config.FullDetails;
                            });```

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry about that @niemyjski! It is a prerelease project so we reserve the right to make breaking changes until it goes stable. We try not to do that, but the overloads being added here are more or less the standard ones across the instrumentation from the core repo and much of contrib so it felt needed.

If you switch to...

b.AddRedisInstrumentation(c => {
                                c.EnrichActivityWithTimingEvents = false;
                                c.SetVerboseDatabaseStatements = config.FullDetails;
                            });

...does that work for you?

Choose a reason for hiding this comment

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

Yes, I found that fix and left the comment above. I just wanted to bring it to attention as it wasn't in the release notes as a breaking change. Thanks again!

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.

5 participants