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

fix: Only add Redis instrumentation if connection found in DI #323

Merged
merged 2 commits into from
Nov 22, 2022

Conversation

MikeGoldsmith
Copy link
Contributor

Which problem is this PR solving?

Apps that do not utilise Redis and register a connection before calling AddAutoInstrumentations fail to send telemetry. This is because even though the AddRedisInstrumentation call has the connection as an optional parameter, an exception is thrown if a connection is not provided and one cannot be found in DI.

This PR updates AddAutoInstrumentations to try to find a Redis connection from DI and only call AddRedisInstrumentation if one is found.

Short description of the changes

  • Only add Redis instrumentation if a registered Redis connection is found using DI

@MikeGoldsmith MikeGoldsmith added type: bug Something isn't working version: bump patch A PR with release-worthy changes and is backwards-compatible. labels Nov 21, 2022
@MikeGoldsmith MikeGoldsmith requested a review from a team November 21, 2022 13:27
@MikeGoldsmith MikeGoldsmith self-assigned this Nov 21, 2022
Copy link
Contributor

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

Hmm, I'm still having trouble even after this change.

I used the aspnetcore example app, and on its own with .AddAspNetCoreInstrumentationWithBaggage() it sends a trace with 2 spans. When I swap that out with .AddAutoInstrumentations(), it does not send any traces.

I enabled diagnostics by setting up a file for OTEL_DIAGNOSTICS.json. In the error log I see the following:

2022-11-21T15:45:01.0061910Z:An exception occurred while retrieving OpenTelemetry Tracer. OpenTelemetry tracing will remain disabled. Exception: '{0}'.{System.InvalidOperationException: StackExchange.Redis IConnectionMultiplexer could not be resolved through application IServiceProvider
   at OpenTelemetry.Trace.TracerProviderBuilderExtensions.<>c__DisplayClass0_0.<AddRedisInstrumentation>b__0(IServiceProvider sp, TracerProviderBuilder builder)

...

@MikeGoldsmith
Copy link
Contributor Author

I followed the same steps as you @JamieDanielson and my app continued to send telemetry - not sure what else was different

Copy link
Contributor

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

Ah! Looks like I may have been using the Nuget-published package instead of the local project reference 🙈

I updated my aspnetcore.csproj to have <ProjectReference Include="..\..\src\Honeycomb.OpenTelemetry.AutoInstrumentations\Honeycomb.OpenTelemetry.AutoInstrumentations.csproj" /> instead of <PackageReference Include="Honeycomb.OpenTelemetry.AutoInstrumentations" Version="0.26.0-beta" /> and indeed, I have a trace with 2 spans!

Looks good!

@MikeGoldsmith MikeGoldsmith merged commit 0260a37 into main Nov 22, 2022
@MikeGoldsmith MikeGoldsmith deleted the mike/redis branch November 22, 2022 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working version: bump patch A PR with release-worthy changes and is backwards-compatible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AddAutoInstrumentations fails due to missing config for AddRedisInstrumentation
3 participants