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

Invoke SDK initialization when using OpenTelemetry.Extensions.Hosting #2901

Merged
merged 4 commits into from
Feb 16, 2022

Conversation

alanwest
Copy link
Member

@alanwest alanwest commented Feb 15, 2022

Fixes #2898

Using the microservice example to demonstrate that the SDK does not get initialized when expected.

The the static constructor on OpenTelemtry.Sdk does not get invoked until after the first message is sent and received in the microservice example:

static Sdk()
{
Propagators.DefaultTextMapPropagator = new CompositeTextMapPropagator(new TextMapPropagator[]
{
new TraceContextPropagator(),
new BaggagePropagator(),
});

@utpilla

@@ -58,7 +58,7 @@ public void StartConsumer()
public void ReceiveMessage(BasicDeliverEventArgs ea)
{
// Extract the PropagationContext of the upstream parent from the message headers.
var parentContext = Propagator.Extract(default, ea.BasicProperties, this.ExtractTraceContextFromBasicProperties);
var parentContext = Propagators.DefaultTextMapPropagator.Extract(default, ea.BasicProperties, this.ExtractTraceContextFromBasicProperties);
Copy link
Member Author

Choose a reason for hiding this comment

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

On the first message received, the DefaultTextMapPropagator is the noop instance. By the second message received it has been initialized to the composite propagator that extracts trace context and baggage.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Resolved this by touching the Sdk class in OpenTelemetry.Extensions.Hosting.

Copy link
Member

Choose a reason for hiding this comment

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

A gentle touch, and SDK behaves!

@codecov
Copy link

codecov bot commented Feb 15, 2022

Codecov Report

Merging #2901 (b006ed8) into main (360957d) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2901      +/-   ##
==========================================
- Coverage   84.20%   84.19%   -0.01%     
==========================================
  Files         252      252              
  Lines        8893     8895       +2     
==========================================
+ Hits         7488     7489       +1     
- Misses       1405     1406       +1     
Impacted Files Coverage Δ
...ensions.Hosting/OpenTelemetryServicesExtensions.cs 71.42% <100.00%> (+2.19%) ⬆️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 71.29% <0.00%> (-0.93%) ⬇️

@alanwest alanwest marked this pull request as ready for review February 16, 2022 01:40
@alanwest alanwest requested a review from a team February 16, 2022 01:40
@alanwest alanwest changed the title Demonstrate goofy SDK initialization Invoke SDK initialization when using OpenTelemetry.Extensions.Hosting Feb 16, 2022
Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

would be good to add changelog.md entry too in hosting package?

@alanwest
Copy link
Member Author

would be good to add changelog.md entry too in hosting package?

Yes, I wavered on this, but I think I came up with something that makes sense.

@cijothomas cijothomas merged commit a2c1b6e into open-telemetry:main Feb 16, 2022
@alanwest alanwest deleted the alanwest/goofiness branch February 23, 2022 19:39
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.

Propagator.Extract not fetching the baggage
4 participants