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

Is only ASP.NET Core supported by Diagnostics libraries? #6367

Closed
jstafford5380 opened this issue Apr 26, 2021 · 39 comments · Fixed by #7195
Closed

Is only ASP.NET Core supported by Diagnostics libraries? #6367

jstafford5380 opened this issue Apr 26, 2021 · 39 comments · Fixed by #7195
Assignees
Labels
api: clouderrorreporting Issues related to the Error Reporting API. api: cloudtrace Issues related to the Cloud Trace API. api: logging Issues related to the Cloud Logging API. type: question Request for information or clarification. Not an issue.

Comments

@jstafford5380
Copy link

The documentation all assumes aspnetcore, but what about worker services? Right now, I'm assuming that I still have to bring in those asp libraries to enable diagnostics, but I cannot use the .UseGoogleDiagnostics() installer since there is no webhostbuilder, and I would instead have to install the products individually. Also, this forces me to bring asp dependencies into the service which is not idea.

@jskeet
Copy link
Collaborator

jskeet commented Apr 27, 2021

The Google.Cloud.Diagnostics.AspNet and Google.Cloud.Diagnostics.AspNetCore packages are integration packages, but they just use the Logging and Trace APIs under the hood - which you can use via the Google.Cloud.Logging.V2 and Google.Cloud.Trace.V2 packages.

Could you give more details about what you'd expect to see? Are you still hoping for an ILoggerProvider, but without other ASP.NET Core dependencies, or are you not using ILogger at all?

@jskeet jskeet added api: logging Issues related to the Cloud Logging API. type: question Request for information or clarification. Not an issue. labels Apr 27, 2021
@jskeet
Copy link
Collaborator

jskeet commented Apr 27, 2021

(Assigned to Amanda as she does more with Diagnostics than I do.)

@amanda-tarafa
Copy link
Contributor

For background services take a look at #4855, it has a Logging working example. Might not be entirely what you are looking for but will probably point you in the right direction. #5018 and #5135 might also help.

As @jskeet the Google.Cloud.Diagnostics.AspNetCore* libraries are integration libraries with ASP.NET Core and heavily depend on ASP.NET Core's pipeline. You can use the API client libraries directly and even Google.Cloud.Diagnostics.Common if you want to implement Diagnostics in other than ASP.NET Core. You might also be able to reuse some of Google.Cloud.Diagnostics.AspNetCore* for that.

Let me know if the info here helps you or if you continue to be blocked, then share more info in what exactly you are attempting to do.

@amanda-tarafa amanda-tarafa added api: clouderrorreporting Issues related to the Error Reporting API. api: cloudtrace Issues related to the Cloud Trace API. needs more info This issue needs more information from the customer to proceed. labels Apr 27, 2021
@amanda-tarafa amanda-tarafa changed the title Is only ASP supported? Is only ASP.NET Core supported by Diagnostics libraries? Apr 27, 2021
@jstafford5380
Copy link
Author

jstafford5380 commented Apr 27, 2021

@jskeet yes we would definitely be using ILogger. I believe a colleague of mine here at PetSmart (whos just went on PTO) was able to figure out an installer that uses IHostBuilder instead of IWebHostBuilder that seems to be inspired from some code that's already in the Google.Diagnostics.AspNetCore package and it appears to be working at the moment but it does rely on the Google.Diagnostics.AspNetCore package. The installer looks something like this, in case it gives any inspiration:

public static class GcpDiagnosticsInstaller
    {
        private const string GoogleProjectIdSection = "Google:ProjectId";
        private const string GoogleLogLevelSection = "Google:LogLevel";
        
        /// <summary>
        /// Adds Google Logging and Tracing capabilities.
        /// </summary>
        /// <param name="builder">An instance of <see cref="IHostBuilder"/></param>
        /// <returns></returns>
        public static IHostBuilder AddGoogleDiagnostics(this IHostBuilder builder)
        {
            return builder.ConfigureServices((context, services) =>
            {
                var settings = GetSettingsFromConfig(context);
                var loggerOpts = LoggerOptions.Create(settings.LogLevel);
                
                services.AddSingleton<ILoggerProvider>(provider => GoogleLoggerProvider.Create(provider, settings.ProjectId, loggerOpts));
                services.AddSingleton<IStartupFilter>(new GcpDiagnosticsStartupFilter());
                services.AddGoogleTrace(opt =>
                {
                    opt.ProjectId = settings.ProjectId;
                });
            });
        }

...
internal class GcpDiagnosticsStartupFilter : IStartupFilter
    {
        public Action<IApplicationBuilder> Configure(Action<IApplicationBuilder> next)
        {
            return app =>
            {
                app.UseGoogleTrace();
                next(app);
            };
        }
    }

Do you see this being as being problematic? I haven't dug too deep into it yet to see if logs and traces were correlating, but logs do seem to be coming through.

@jskeet
Copy link
Collaborator

jskeet commented Apr 28, 2021

I'll wait for @amanda-tarafa to comment as she knows the code better than I do. My initial concern would be around buffering of log output.

@amanda-tarafa
Copy link
Contributor

Logging should be fine.

Tracing I'm not sure it is working. When you do app.UseGoogleTrace() you are just setting up the ASP.NET Core Google Trace Middleware, which will only trigger for HTTP requests to your app (which if I understand correctly, you don't have since this is a background service). The Google Trace Middleware is the one that correctly initializes the tracer, so I'm pretty sure you are ending up with a NullManagedTracer which basically does nothing.
Again, take a look at my example on #4855, in particular at QueuedHostedService so you can see how to initialize the tracer in the absence of the Google Trace Middleware. You can remove app.UseGoogleTrace() entirely, but do leave the service configuration bits so you have everything set up properly.

@jstafford5380
Copy link
Author

Ok I will take a look at that specifically once I find a moment, I downloaded the zip. At least we know logging is good.

@jstafford5380
Copy link
Author

jstafford5380 commented May 4, 2021

@amanda-tarafa Finally got back to this. Ok so the example in the other thread appears to just fall back to using an ASP project type instead of the worker, and then using .AddHostedService<> to host the background service. Is that right? Or am I just looking at the pattern to establish the tracer?

@jstafford5380
Copy link
Author

jstafford5380 commented May 4, 2021

It looks like this is actually creating a NullManagedTracer

image

It actually appears to always give a NullManagedTracer if there's no HttpContext. I recreated as asp project and if I do a trace from the controller, it works fine, but if I do it from the background worker, I get NullManagedTracer. I see yours is doing it in the background, so I'm not sure what the difference is unless you're inadvertently capturing the http context or something when you do the enqueue
.

@amanda-tarafa
Copy link
Contributor

Or am I just looking at the pattern to establish the tracer?

Yes, look only at how to establish the tracer, i.e. the code in QueuedHostedService, the other person was working on a BackgroundService in ASP.NET Core, but how to set the tracer is the same in the absence of HttpContext.

It actually appears to always give a NullManagedTracer if there's no HttpContext.

No that's not why, this way of creating the tracer is not dependendt on HttpContext. You are getting a NullManagedTracer because of the tracing frequency you have configured tracing with. You can check that by either setting a very high QPS, or if you want to always force tracing of the "offline" processes, you can create the tracer like this, passing true to shouldTrace.

var tracer = _tracerFactory(TraceHeaderContext.Create(null, null, true));

Let me know.

@jstafford5380
Copy link
Author

jstafford5380 commented May 5, 2021

@amanda-tarafa nice to meet you and Jon today! I went back and got things set up the way we talked about and initially, the logs and traces were coming through and correlating correctly, so I moved them back into position for what would be the final implementation and then things broke again.

The difference between the test and the final implementation is that the code that is used to establish the trace context happens in a delegate:

image
image

Above, the reachable code works. If I comment that part out to make the rest reachable, it no longer works. If I step through the part that isn't working, I can see I'm getting a SimpleManagedTracer so I'm not sure what the hangup is.

The idea here is any time a message is picked up from Azure Service Bus, the ReceiveMessage callback is invoked and that creates it's own trace context.

As for setup:

options.Options = TraceOptions.Create(
      qpsSampleRate: double.MaxValue,
      bufferOptions: BufferOptions.NoBuffer(),
      retryOptions: RetryOptions.NoRetry(ExceptionHandling.Propagate));

And I have something similar for the logging options.

When I find some time, I might be able to get a repro put together, but I wanted to give you a head start in case there was something obvious that I'm missing.

@amanda-tarafa
Copy link
Contributor

Same here, nice to e-meet you :).

Nothing inmediately jumps out at me. I will try to recreate what I think you are doig but it would be useful if you could share the code that calls the processors, i.e., the code that receives the messages. (I'm also not certain what classes are from Azure libraries and what are yours).

Things to check:

  • How many functions have you attached to processors.ProcessMessageAsync? Could it be that the one that starts the tracer is not the first one to execute?
  • Could it be that something is silently failing before reaching the ReceiveMessage handler?

I'll try to reproduce this and report back early next week, if you can get a repro together that will probably make things faster.

@jstafford5380
Copy link
Author

How many functions have you attached to processors.ProcessMessageAsync? Could it be that the one that starts the tracer is not the first one to execute?

  • ReceiveMessage is the delegate, and it is the one the starts the tracer, so it should be separate for every incoming message. I'm not 100% sure that those are isolated contexts (C# event handlers/multicast delegates), but I assume it is. Even if they weren't, I'd expect to see conflicting traces, but I'm not getting any at all.

Could it be that something is silently failing before reaching the ReceiveMessage handler?
I don't think so. The entire receiver code is running and messages getting processed, just no traces nor logs. There's nothing in Visual Studio Diagnostics that indicate any failures.

Maybe service scope is an issue. I'll try to create an example that mimicks this setup.

@jstafford5380
Copy link
Author

jstafford5380 commented May 10, 2021

So I was able to create what I felt was a decent facsimile of all the components involved in my implementation, all the way up to the point the delgate/handler was registered. The only real difference that I can see is that my example doesn't use Azure.Messaging.ServiceBus whereas my real code does.

My facsimile actually works correctly. I then tried swapping the fake implementation into my real code and it started working again... so this is starting to lead me to believe that there may be something with how Azure.Messaging.ServiceBus handles those delegates?

fake container

public void CreateProcessors(
    ServiceBusProcessorOptions options, 
    Func<ProcessMessageEventArgs, Task> receiver, 
    Func<ProcessErrorEventArgs, Task> errorHandler)
{
    for (var i = 0; i < 4; i++) // create 4 fake processors
    {
        var processor = new FakeProcessor();
        processor.MessageReceived += receiver;
        _processors.Add(processor);
    }
}

public Task StartProcessorsAsync()
{
    _isRunning = true;

    // simulate incoming messages.
    Task.Run(async () =>
    {
        while (_isRunning)
        {
            await Task.Delay(_rand.Next(25, 300)); // simulate messages coming in at random
            foreach (var processor in _processors)
            {
                await processor.SendMessage($"Heartbeat {Guid.NewGuid():N}");
            }
        }
    }).FireAndForget();
    return Task.CompletedTask;
}

actual container

public void CreateProcessors(
    ServiceBusProcessorOptions options,
    Func<ProcessMessageEventArgs, Task> receiver,
    Func<ProcessErrorEventArgs, Task> errorHandler)
{
    _logger.LogInformation("Creating processors for {processorCount} subscriptions...", _subscriptions.Count);

    foreach (var subscription in _subscriptions)
    {
        var processor = _client.CreateProcessor(subscription.Topic, subscription.Subscription, options);
        processor.ProcessMessageAsync += receiver;
        processor.ProcessErrorAsync += errorHandler;
        _processors.Add(processor);
    }
}

public async Task StartProcessorsAsync()
{
    _logger.LogInformation("Starting {processorCount} processors...", _processors.Count);

    foreach (var processor in _processors)
        await processor.StartProcessingAsync();
}

Receive method that is the delegate which gets called when a message comes in
I just comment out some of the message values when using the fake container

private async Task ReceiveMessage(ProcessMessageEventArgs args)
{
    const string emptySubject = "Empty";
    var label = args.Message?.Subject ?? emptySubject;

    var tracer = _tracerFactory(TraceHeaderContext.Create(null, null, true));
    ContextTracerManager.SetCurrentTracer(tracer);
    using var span = tracer.StartSpan($"{args.Message?.MessageId ?? "NoLabel"}_{label}");

    
    if (label == emptySubject)
    {
        _logger.LogError("Incoming message did not have a label/subject! Sending to dead letter...");
        _logger.LogDebug("Message missing label. Message Body: {messageBody} Message Headers: {messageHeaders}",
            args.Message?.Body?.ToString() ?? "NoBody",
            JsonSerializer.Serialize(args.Message?.ApplicationProperties ?? new Dictionary<string, object>()));

        await args.DeadLetterMessageAsync(args.Message, deadLetterReason: "MissingLabel", deadLetterErrorDescription: "No label was contained on the message, so we didn't know how to route it!");
        return;
    }

    if(!_eventMap.TryGetAdapter(label, out var adapter))
    {
        _logger.LogError("There was no registration for the label '{label}'. Sending to dead letter", label);
        await args.DeadLetterMessageAsync(args.Message, deadLetterReason: "MissingAdapterRegistration", deadLetterErrorDescription: $"The label '{label}' was not registered in EventMapping.cs");
        return;
    }

    await adapter.AdaptAndRepublish(args.Message, args.CancellationToken);
    await args.CompleteMessageAsync(args.Message);
}

I can share you the real code but it has some private packages in it and needs azure service bus to run the broken version. The code I put together to actually reproduce the problem turned out to work, so I don't think that's going to be much help.

@amanda-tarafa amanda-tarafa removed the needs more info This issue needs more information from the customer to proceed. label May 11, 2021
@amanda-tarafa
Copy link
Contributor

I won't have time to look at this today, but I'll look this week. We'll figure it out :)

@amanda-tarafa
Copy link
Contributor

Just a quick update: Same as you I haven't been able to reproduce any problems with the code as you pasted it. Today I'll try to tweak it and see if I get to a point where I can reproduce lack of traces.

@amanda-tarafa
Copy link
Contributor

OK, a few more questions, because I haven't been able to reproduce any issues:

  • Can you share the FakeProcessor code with me as well? I've made some simple assumptions there, but just in case. I'm trying to think about the differences of your fake code vs. what Azure libraries might be doing.
  • Can you configure tracing like this, basically, no buffer, no retry and exception propagation, on the code that you are running on Azure. Just in case there's an exception somewhere.

amanda-tarafa added a commit to amanda-tarafa/google-cloud-dotnet that referenced this issue May 26, 2021
amanda-tarafa added a commit to amanda-tarafa/google-cloud-dotnet that referenced this issue May 26, 2021
amanda-tarafa added a commit to amanda-tarafa/google-cloud-dotnet that referenced this issue May 26, 2021
amanda-tarafa added a commit to amanda-tarafa/google-cloud-dotnet that referenced this issue May 26, 2021
jskeet added a commit to jskeet/google-cloud-dotnet that referenced this issue Jun 24, 2021
Changes in Google.Cloud.Diagnostics.AspNetCore version 4.3.0-beta01:

- [Commit 60e8cd8](googleapis@60e8cd8):
  - feat: Copies GoogleLogger to Common. This allows easier use of GoogleLogger in non ASP.NET Core applications.
  - Towards [issue 6367](googleapis#6367)
  - Replicate LoggerOptions in Common, and have AspNetCore\*.LoggerOptions be just a wrapper of Common.LoggerOptions.
  - Copies ILogEntryLabelProvider to Common and marks the one in AspNetCore* Obsolete.
  - Makes AspNetCore*.IExternalTraceProvider obsolete. It can now be replaced by Common.ITraceContext.
- [Commit 32cb606](googleapis@32cb606):
  - feat: Allows per log entry labels.
  - Closes [issue 5313](googleapis#5313)
  - Closes [issue 5929](googleapis#5929)
- [Commit c8e9a48](googleapis@c8e9a48):
  - refactor: Makes GoogleLoggerScope extendable.
    Moves GoogleLoggerScope to Diagnostics.Common.
    In preparation for allowing LogEntry augmentation and making it easier to use Google logging from non ASP.NET Core apps.
    Towards [issue 5313](googleapis#5313), [issue 5360](googleapis#5360), [issue 5929](googleapis#5929) and [issue 6367](googleapis#6367)
- [Commit 7f5f89e](googleapis@7f5f89e):
  - docs: Change Stackdriver to Google Cloud, and fix some typos, including in test code.
- [Commit c4c9cd5](googleapis@c4c9cd5):
  - feat: Makes it easier to use tracing from non ASP.NET Core applications
    Closes [issue 5897](googleapis#5897)
    Towards [issue 6367](googleapis#6367)
- [Commit b35b9ea](googleapis@b35b9ea):
  - feat: Decouples Diagnostics tracing from Google's trace header. Towards [issue 5360](googleapis#5360) and [issue 5897](googleapis#5897)
- [Commit 0c00d2c](googleapis@0c00d2c):
  - refactor: Remove unnecesary service provider extension method. There's an equivalent method offered by Microsoft.Extensions.DependencyInjection so we don't need our own.
- [Commit bb0c7b2](googleapis@bb0c7b2):
  - refactor: Remove unnecesary interface IManagedTracerFactory. It's an internal interface that we don't use anywhere. We can always add it back in if we need it at some point.
- [Commit 8ef3983](googleapis@8ef3983):
  - fix: X-Cloud-Trace-Context trace mask values should be 0-1. See https://cloud.google.com/trace/docs/setup#force-trace

Note: changing a generic type parameter constraint in
`LabelProviderExtensions` is notionally a breaking change, but due
to how it will be used, we don't expect it to actually break any customers.

Changes in Google.Cloud.Diagnostics.AspNetCore3 version 4.3.0-beta01:

- [Commit 60e8cd8](googleapis@60e8cd8):
  - feat: Copies GoogleLogger to Common. This allows easier use of GoogleLogger in non ASP.NET Core applications.
  - Towards [issue 6367](googleapis#6367)
  - Replicate LoggerOptions in Common, and have AspNetCore\*.LoggerOptions be just a wrapper of Common.LoggerOptions.
  - Copies ILogEntryLabelProvider to Common and marks the one in AspNetCore* Obsolete.
  - Makes AspNetCore*.IExternalTraceProvider obsolete. It can now be replaced by Common.ITraceContext.
- [Commit 32cb606](googleapis@32cb606):
  - feat: Allows per log entry labels.
  - Closes [issue 5313](googleapis#5313)
  - Closes [issue 5929](googleapis#5929)
- [Commit c8e9a48](googleapis@c8e9a48):
  - refactor: Makes GoogleLoggerScope extendable.
    Moves GoogleLoggerScope to Diagnostics.Common.
    In preparation for allowing LogEntry augmentation and making it easier to use Google logging from non ASP.NET Core apps.
    Towards [issue 5313](googleapis#5313), [issue 5360](googleapis#5360), [issue 5929](googleapis#5929) and [issue 6367](googleapis#6367)
- [Commit 7f5f89e](googleapis@7f5f89e):
  - docs: Change Stackdriver to Google Cloud, and fix some typos, including in test code.
- [Commit c4c9cd5](googleapis@c4c9cd5):
  - feat: Makes it easier to use tracing from non ASP.NET Core applications
    Closes [issue 5897](googleapis#5897)
    Towards [issue 6367](googleapis#6367)
- [Commit b35b9ea](googleapis@b35b9ea):
  - feat: Decouples Diagnostics tracing from Google's trace header. Towards [issue 5360](googleapis#5360) and [issue 5897](googleapis#5897)
- [Commit 0c00d2c](googleapis@0c00d2c):
  - refactor: Remove unnecesary service provider extension method. There's an equivalent method offered by Microsoft.Extensions.DependencyInjection so we don't need our own.
- [Commit bb0c7b2](googleapis@bb0c7b2):
  - refactor: Remove unnecesary interface IManagedTracerFactory. It's an internal interface that we don't use anywhere. We can always add it back in if we need it at some point.
- [Commit 8ef3983](googleapis@8ef3983):
  - fix: X-Cloud-Trace-Context trace mask values should be 0-1. See https://cloud.google.com/trace/docs/setup#force-trace

Note: changing a generic type parameter constraint in
`LabelProviderExtensions` is notionally a breaking change, but due
to how it will be used, we don't expect it to actually break any customers.

Packages in this release:
- Release Google.Cloud.Diagnostics.AspNetCore version 4.3.0-beta01
- Release Google.Cloud.Diagnostics.AspNetCore3 version 4.3.0-beta01
- Release Google.Cloud.Diagnostics.Common version 4.3.0-beta01
jskeet added a commit that referenced this issue Jun 24, 2021
Changes in Google.Cloud.Diagnostics.AspNetCore version 4.3.0-beta01:

- [Commit 60e8cd8](60e8cd8):
  - feat: Copies GoogleLogger to Common. This allows easier use of GoogleLogger in non ASP.NET Core applications.
  - Towards [issue 6367](#6367)
  - Replicate LoggerOptions in Common, and have AspNetCore\*.LoggerOptions be just a wrapper of Common.LoggerOptions.
  - Copies ILogEntryLabelProvider to Common and marks the one in AspNetCore* Obsolete.
  - Makes AspNetCore*.IExternalTraceProvider obsolete. It can now be replaced by Common.ITraceContext.
- [Commit 32cb606](32cb606):
  - feat: Allows per log entry labels.
  - Closes [issue 5313](#5313)
  - Closes [issue 5929](#5929)
- [Commit c8e9a48](c8e9a48):
  - refactor: Makes GoogleLoggerScope extendable.
    Moves GoogleLoggerScope to Diagnostics.Common.
    In preparation for allowing LogEntry augmentation and making it easier to use Google logging from non ASP.NET Core apps.
    Towards [issue 5313](#5313), [issue 5360](#5360), [issue 5929](#5929) and [issue 6367](#6367)
- [Commit 7f5f89e](7f5f89e):
  - docs: Change Stackdriver to Google Cloud, and fix some typos, including in test code.
- [Commit c4c9cd5](c4c9cd5):
  - feat: Makes it easier to use tracing from non ASP.NET Core applications
    Closes [issue 5897](#5897)
    Towards [issue 6367](#6367)
- [Commit b35b9ea](b35b9ea):
  - feat: Decouples Diagnostics tracing from Google's trace header. Towards [issue 5360](#5360) and [issue 5897](#5897)
- [Commit 0c00d2c](0c00d2c):
  - refactor: Remove unnecesary service provider extension method. There's an equivalent method offered by Microsoft.Extensions.DependencyInjection so we don't need our own.
- [Commit bb0c7b2](bb0c7b2):
  - refactor: Remove unnecesary interface IManagedTracerFactory. It's an internal interface that we don't use anywhere. We can always add it back in if we need it at some point.
- [Commit 8ef3983](8ef3983):
  - fix: X-Cloud-Trace-Context trace mask values should be 0-1. See https://cloud.google.com/trace/docs/setup#force-trace

Note: changing a generic type parameter constraint in
`LabelProviderExtensions` is notionally a breaking change, but due
to how it will be used, we don't expect it to actually break any customers.

Changes in Google.Cloud.Diagnostics.AspNetCore3 version 4.3.0-beta01:

- [Commit 60e8cd8](60e8cd8):
  - feat: Copies GoogleLogger to Common. This allows easier use of GoogleLogger in non ASP.NET Core applications.
  - Towards [issue 6367](#6367)
  - Replicate LoggerOptions in Common, and have AspNetCore\*.LoggerOptions be just a wrapper of Common.LoggerOptions.
  - Copies ILogEntryLabelProvider to Common and marks the one in AspNetCore* Obsolete.
  - Makes AspNetCore*.IExternalTraceProvider obsolete. It can now be replaced by Common.ITraceContext.
- [Commit 32cb606](32cb606):
  - feat: Allows per log entry labels.
  - Closes [issue 5313](#5313)
  - Closes [issue 5929](#5929)
- [Commit c8e9a48](c8e9a48):
  - refactor: Makes GoogleLoggerScope extendable.
    Moves GoogleLoggerScope to Diagnostics.Common.
    In preparation for allowing LogEntry augmentation and making it easier to use Google logging from non ASP.NET Core apps.
    Towards [issue 5313](#5313), [issue 5360](#5360), [issue 5929](#5929) and [issue 6367](#6367)
- [Commit 7f5f89e](7f5f89e):
  - docs: Change Stackdriver to Google Cloud, and fix some typos, including in test code.
- [Commit c4c9cd5](c4c9cd5):
  - feat: Makes it easier to use tracing from non ASP.NET Core applications
    Closes [issue 5897](#5897)
    Towards [issue 6367](#6367)
- [Commit b35b9ea](b35b9ea):
  - feat: Decouples Diagnostics tracing from Google's trace header. Towards [issue 5360](#5360) and [issue 5897](#5897)
- [Commit 0c00d2c](0c00d2c):
  - refactor: Remove unnecesary service provider extension method. There's an equivalent method offered by Microsoft.Extensions.DependencyInjection so we don't need our own.
- [Commit bb0c7b2](bb0c7b2):
  - refactor: Remove unnecesary interface IManagedTracerFactory. It's an internal interface that we don't use anywhere. We can always add it back in if we need it at some point.
- [Commit 8ef3983](8ef3983):
  - fix: X-Cloud-Trace-Context trace mask values should be 0-1. See https://cloud.google.com/trace/docs/setup#force-trace

Note: changing a generic type parameter constraint in
`LabelProviderExtensions` is notionally a breaking change, but due
to how it will be used, we don't expect it to actually break any customers.

Packages in this release:
- Release Google.Cloud.Diagnostics.AspNetCore version 4.3.0-beta01
- Release Google.Cloud.Diagnostics.AspNetCore3 version 4.3.0-beta01
- Release Google.Cloud.Diagnostics.Common version 4.3.0-beta01
@amanda-tarafa
Copy link
Contributor

@jstafford5380 Have you had any chance trying out our latest beta releases? They include features to have the Diagnostics libraries work from non ASP.NET Core apps. Also, I wonder if the errors you were seeing in Azure could be fixed by using the new versions.

I'm keeping this issue open because I want to improve the docs around using the libraries outside of ASP.NET Core, but I'll close it as soon as I've done that. If you continue to experiment issues in Azure, we can open a new issue just focusing on that. Let me know.

@jstafford5380
Copy link
Author

I have not tried it yet, but I will attempt to do so this week, thanks!

@amanda-tarafa
Copy link
Contributor

Thanks! And if I close the issue, we can always reopen, is jut so that it doesn't catch my attention while there's nothing I can do on my side.

@idofl
Copy link

idofl commented Jul 28, 2021

I just spent the past week testing the new 4.3.0-beta03. I tested both ASP.NET Core and a BackgroundService that listens to Google PubSub (without any *.aspnetcore library, only Google.Cloud.Diagnostics.Common).
I was able to use both Tracing and Logging in both types of apps, and I was able to create a distributed trace that spans ASP.NET and PubSub, including reusing the trace ID for all related logs.
@jstafford5380 let me know if you have any questions on how to get the distributed trace/log working.

@amanda-tarafa
Copy link
Contributor

That's good to know @idofl . I'll be shortly updating the docs describing these new features, and given that you've already used them it'll be great if I get your input on the PR, so I'll flag you there (eow most likely).

@jstafford5380
Copy link
Author

jstafford5380 commented Aug 22, 2021

@idofl yeah I could use a hand. I'm looking at it right now and trying to figure out what I'm supposed to do. So far, I am doing the following in my test installer:

public static void InstallGoogleDiagnostics(this IServiceCollection services, IConfiguration config)
{
    services.AddHttpContextAccessor();

    CloudTraceExtensions.AddGoogleTrace(services, options =>
    {
        options.ProjectId = config["Google:ProjectId"];
        options.Options = TraceOptions.Create(double.MaxValue, BufferOptions.NoBuffer(), RetryOptions.NoRetry(ExceptionHandling.Propagate));
    });

    services.AddScoped(ProvideZipkinTraceContext);
    services.AddSingleton<Action<HttpResponse, ITraceContext>>((response, traceContext) =>
        response.Headers.Add("some_header", traceContext.TraceId));
}

private static ITraceContext ProvideZipkinTraceContext(IServiceProvider serviceProvider)
{
    var httpContext = serviceProvider.GetRequiredService<IHttpContextAccessor>().HttpContext;
    var adapter = ZipkinTraceAdapter.FromHeaders(httpContext.Request.Headers);
    return adapter;
}

I'm not getting any errors but the ProvideZipkinTraceContext code never gets hit.

@amanda-tarafa
Copy link
Contributor

Make sure you are calling Diagnostics.AspNetCore3.CloudTraceExtension.AddGoogleTrace(...) instead of Diagnostics.Common.CloudTraceExtensions.AddGoogleTrace(...) which seems to be what you are calling. It should work fine after that.

Diagnostics.Common.CloudTraceExtensions.AddGoogleTrace(...) won't set up the ASP.NET Core middleware (because it is ASP.NET Core agnostic), and it's the middleware what kick starts the whole process.

See #6672 for the explanation of why Common.CloudTraceExtension**s** vs. AspNetCore3.CloudTraceExtension.

@idofl
Copy link

idofl commented Aug 23, 2021

@jstafford5380 , as Amanda wrote, in non-ASP apps, use the Google.Cloud.Diagnostics.Common. CloudTraceExtensions .AddGoogleTrace. It's available in the latest beta version:
https://www.nuget.org/packages/Google.Cloud.Diagnostics.Common/4.3.0-beta03

  1. Call AddGoogleTrace in your program.cs, just like you do with an ASPNET app
  2. In your worker (derived from BackgroundService) adjust it to receive the tracer factory function:
public Worker(Func<ITraceContext, IManagedTracer> tracerFactory)
{       
    _tracerFactory = tracerFactory;
}
  1. In your worker, initialize the trace context (code below uses a custom attribute that was attached to a pubsub message):
private void InitializeTracingFromMessage(PubsubMessage message)
{
    // Extract trace information from the message attributes
    string traceContextAttribute = message.Attributes["custom-trace-context"];
    
    // Parse the trace context header
    ITraceContext context = TraceHeaderContext.FromHeader(traceContextAttribute);
    
    // Create the IManagedTracer for the current trace context
    var tracer = _tracerFactory(context);
    
    // Set current tracer for the DI when asked for IManagedTracer in the scoped service
    ContextTracerManager.SetCurrentTracer(tracer);
}
  1. From here, there are two options to create new spans:
  • Store the tracer globally, and call tracer.StartSpan - You might want this to be thread/async local
  • Call ContextTracerManager.GetCurrentTracer().StartSpan - The class handles thread safety therefore the recommended way
  • After calling InitializeTracingFromMessage, create a new scoped instance of your actual service, and add IManagedTracer to its constructor. From here on continue similar to how you use ASP.NET controllers.
    (see here how to create scoped services in a background service)

To store the trace context in a non-HTTP scenario, use the following code to get the trace context:

ITraceContext context = ContextTracerManager.GetCurrentTraceContext();
ITraceContext traceHeaderContext = TraceHeaderContext.Create(context.TraceId, context.SpanId ?? 0, context.ShouldTrace);
var traceContextValue = traceHeaderContext.ToString();

In the above code snippets, I stored the value of traceContextValue in the pubsub message.

I hope this helps.

@amanda-tarafa
Copy link
Contributor

@jstafford5380 Just to make it clearer:

First:

Make sure you are calling Diagnostics.AspNetCore3.CloudTraceExtension.AddGoogleTrace(...) instead of Diagnostics.Common.CloudTraceExtensions.AddGoogleTrace(...) which seems to be what you are calling. It should work fine after that.

This part of my answer applies if you are extracting trace context information from HTTP Requests (which seems to be what you were trying to do in your code).

Second:
@idofl 's whole answer applies if you want to extract context information from non-HTTP requests, like Pub/Sub receives or similar.

@jstafford5380
Copy link
Author

jstafford5380 commented Aug 23, 2021

Yeah it looks like we have a few too many related topics all in flight at the same time. At this point in time, I am exploring the changes that allow me to use a non-google trace headers (e.g. B3) in an ASP context. If I do it using what Ido described above, I get the traces but without any of the ASP-injected info. So that looks useful for our workers, but at the moment, I'm trying to adapt b3 headers coming from apigee and anthos.

I was digging through some of the commits looking for how to achieve this, and it looked I needed to replace some stuff in DI which is what I pasted above, replacing ITraceContext and adding that action to DI for responses but the code never gets hit. I feel like I'm missing a registration of a new factory or something

@jstafford5380
Copy link
Author

Also worth noting that using Ido's example, none of the logs were correlated.

@idofl
Copy link

idofl commented Aug 23, 2021

@jstafford5380 Getting logs to correlate is quite straightforward. Add cloud logging in program.cs:

Host.CreateDefaultBuilder(args).ConfigureLogging((hostBuilder, logging) =>
    {
        logging.AddProvider(GoogleLoggerProvider.Create(serviceProvider: null, ProjectId));
    });

When the Cloud Logging provider is invoked, it will call the ContextTracerManager to get the current trace context, and will add the trace id and span id to the log record.

If you rely on console logging and want all console logs to get correlated, then you need to use .NET 5 and create a custom console logger formatter that adheres to the logging json schema. We have an example for that which shows how to successfully correlate logs and traces in GKE, GCF, and Cloud Run through console logging.

@jstafford5380
Copy link
Author

Oh right! Sorry about that, I forgot that's a separate setup (been off this for too long). Ok so that correlates now. Just the other thing remaining - custom trace ids in ASP

@idofl
Copy link

idofl commented Aug 23, 2021

@jstafford5380 For custom trace ids, look at the sample here:

It shows how to add a custom header to the request and response, and how to use it to initialize the trace context.
I've used those successfully to force Cloud Trace to use the .NET Core Activity trace ids.

BTW, if you host your application behind the google cloud HTTP(S) load balancer, the LB adds the traceparent and x-cloud-trace-context headers to each incoming request and set both headers to the same value, so both Cloud Trace and the Activity object are initialized with the same trace id. If you are using a trace mechanism that looks for the traceparent header in incoming requests, then you might not need to manually change the trace id for cloud trace, as it will already have the same value and will use it when propagating the trace through new http requests.

@jstafford5380
Copy link
Author

jstafford5380 commented Aug 23, 2021

We are behind a google lb but it is not inserting a traceparent. However, it does insert the b3 headers and x-cloud-trace-context so if we leave it alone, the sdk correlates correctly by default, but we're actually wanting to implement the w3c traceparent longer term. It seems like it would be easy convert between that and b3 headers which is why i was experimenting with it.

This does make it challenging though because our service bus stuff is on the new traceparent format. So if we use x-cloud-trace-context then when re-establishing a trace that uses traceparent... is their a sane way to convert between the two? Maybe just use the traceid (32 hex chars) and convert the parentid to integer? That would probably work in a custom formatter.

@jstafford5380 For custom trace ids, look at the sample here:
google-cloud-dotnet/apis/Google.Cloud.Diagnostics.AspNetCore/Google.Cloud.Diagnostics.AspNetCore.Snippets/TraceSnippets.cs

Line 372 in c4c9cd5

public void ConfigureServices(IServiceCollection services)
It shows how to add a custom header to the request and response, and how to use it to initialize the trace context.
I've used those successfully to force Cloud Trace to use the .NET Core Activity trace ids.

Ok so then I was looking in a correct place and that's what I based my code on. I can use tracer factory like you said in a non-http context, but the code never runs in an http context.

@idofl
Copy link

idofl commented Aug 24, 2021

@jstafford5380 You are correct regarding LB. I saw this behavior in our serverless and assumed both headers got added by the LB. The traceparent was probably added by the serverless framework.

I can use tracer factory like you said in a non-http context, but the code never runs in an http context.

When you say the code doesn't run in an http context, which part of the code do you mean? the delegate that extracts the custom trace header or the one that places the custom trace id in the request/response headers?

For adding the header - scroll down that file, there is an example how to create an HttpClient using an HttpClient factory. That worked for me when I tested it.

@amanda-tarafa
Copy link
Contributor

I want to go back a few comments from @jstafford5380 :

At this point in time, I am exploring the changes that allow me to use a non-google trace headers (e.g. B3) in an ASP context. If I do it using what Ido described above, I get the traces but without any of the ASP-injected info. So that looks useful for our workers, but at the moment, I'm trying to adapt b3 headers coming from apigee and anthos.

What @idofl described is only needed in non ASP.NET Core contexts (I'm guessing that when you say ASP, you really mean ASP.NET Core). You can get it to work, but you'll end up writing a lot of code that's already in Google.Cloud.Diagnostics.AspNetCore*. The code that you pasted initially is almost correct, and the only thing you need to do is change the way you are registering Trace (and register logging if you want to log entries to be correlated). Your code would look like this:

public static void InstallGoogleDiagnostics(this IServiceCollection services, IConfiguration config)
{
    // No need to add the HTTP Context accesor, we do it already.

    // Make sure this is the one defined in Google.Cloud.Diagnostics.AspNetCore(3).
    // Notice that the class in Google.Cloud.Diagnostics.AspNetCore(3) is named without an s at the end.
    Google.Cloud.Diagnostics.AspNetCore3.CloudTraceExtension.AddGoogleTrace(services, options =>
    {
        options.ProjectId = config["Google:ProjectId"];
        options.Options = TraceOptions.Create(double.MaxValue, BufferOptions.NoBuffer(), RetryOptions.NoRetry(ExceptionHandling.Propagate));
    });

    services.AddScoped(ProvideZipkinTraceContext);
    services.AddSingleton<Action<HttpResponse, ITraceContext>>((response, traceContext) =>
        response.Headers.Add("some_header", traceContext.TraceId));
}

private static ITraceContext ProvideZipkinTraceContext(IServiceProvider serviceProvider)
{
    var httpContext = serviceProvider.GetRequiredService<IHttpContextAccessor>().HttpContext;
    var adapter = ZipkinTraceAdapter.FromHeaders(httpContext.Request.Headers);
    return adapter;
}

// And then, you need to configure the middleware on your Startup.cs or similar.
Google.Cloud.Diagnostics.AspNetCore(3)
public void Configure(IApplicationBuilder app, ...)
{
    // Use at the start of the request pipeline to ensure the entire
    // request is traced.
    app.UseGoogleTrace();
    ///     ...
}

@amanda-tarafa
Copy link
Contributor

On a separate note, I think this issue is very overloaded and at the point where it's not easy to follow, even for all of us that have been on it from the beginning.
@jstafford5380 , @idofl , how would you feel about leaving this issue as is (I'll leave open until I've finished adding the documentation and examples for these features). @jstafford5380 you can then create one issue per use case you want to solve and are having trouble with (for instance, one for non Google headers in ASP.NET Core context, and one for Pub/Sub listeners) and we can go from there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: clouderrorreporting Issues related to the Error Reporting API. api: cloudtrace Issues related to the Cloud Trace API. api: logging Issues related to the Cloud Logging API. type: question Request for information or clarification. Not an issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants