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

Application Insights Tracing Context Lost on Message Receive #516

Open
jglien opened this issue Mar 23, 2022 · 2 comments
Open

Application Insights Tracing Context Lost on Message Receive #516

jglien opened this issue Mar 23, 2022 · 2 comments

Comments

@jglien
Copy link

jglien commented Mar 23, 2022

Describe the bug
Using the native NServiceBus Pub/Sub pattern in two separate asp.net apps, when messages are received, we see that the Azure Service Bus SDK (presumably) has added the Diagnostic-Id message header, however this header appears to be ignored and Activity.Current is null.

NServiceBus uses Microsoft's SDK, according to Microsoft Docs, this should work out-of-the-box.

Official documentation suggests implementing your own tracing, which would be appropriate for a non-asp.net service, of course, but looks like it won't play nice with the the asp.net App Insights configuration (via .AddApplicationInsightsTelemetry()).

To Reproduce
Use the native NServiceBus Pub/Sub pattern with two asp.net apps using application insights via .AddApplicationInsightsTelemetry(). Have code handling Https request in first app send a message, observe addition of Diagnostic-Id message header on receiving end in second app. Observe operation ID set in senders app insights logs, observe operation ID not automagically applied on receivers logs. Observe Activity.Current is null on receiving end.

Expected behavior
Application Insights logs on both ends should automagically have trace context applied via Activity.Current.

Versions:

  • NuGet package: 2.0.2
  • OS: Docker: Alpine Linux and MacOS
  • .NET Version net6.0
@jglien
Copy link
Author

jglien commented Mar 23, 2022

It is possible to do this the following, but shouldn't be necessary.

public async Task Handle(MyEvent @event, IMessageHandlerContext context)
{
	var activity = Activity.Current;
	if (activity == null && context.MessageHeaders.TryGetValue("Diagnostic-Id", out var objectId) && objectId is string diagnosticId)
	{
		activity = new Activity("NServiceBus.Receive");
		activity.SetParentId(diagnosticId);
	}

	if (activity == null)
	{
		_logger.LogWarning("No Activity found, tracing will not work :(");
		await ProcessMessageAsync(@event);
	}
	else
	{
		using (var operation = _telemtryClient.StartOperation<RequestTelemetry>(activity))
		{
			await ProcessMessageAsync(@event);
		}
	}
}

@ramonsmits
Copy link
Member

ramonsmits commented Mar 24, 2022

@jglien We do not yet support OpenTelemetry. A community contribution exists maintained by @jbogard which is available at https://github.com/jbogard/NServiceBus.Extensions.Diagnostics

However, here you run into the problem that the header keys might be incompatible. I did a quick search on Diagnostic-Id and
It might already resolve your current issue.

The magic happens at https://github.com/jbogard/NServiceBus.Extensions.Diagnostics/blob/master/src/NServiceBus.Extensions.Diagnostics/IncomingPhysicalMessageDiagnostics.cs#L48

This is a type of "native integration" with Azure Service Bus. OpenTelemetry diagnostics identifiers are added and maintained by Azure Service Bus or by NServiceBus.Extensions.Diagnostics and these can be incompatible or conflicting.

In that regard it is likely wise to only have OpenTelemetry enabled on NServiceBus (as that will always propagate into the configured transport) or the native messaging client. However, its up to each native client how they propagate the identifiers.

@ramonsmits ramonsmits self-assigned this Mar 24, 2022
@ramonsmits ramonsmits removed their assignment Jun 27, 2024
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

No branches or pull requests

2 participants