Skip to content

logging changes #123

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

Merged
merged 8 commits into from
Feb 18, 2021
Merged

logging changes #123

merged 8 commits into from
Feb 18, 2021

Conversation

brettsam
Copy link
Member

@brettsam brettsam commented Feb 12, 2021

A couple of notes about the approach here. I think that we should follow a similar pattern to gRPC: https://docs.microsoft.com/en-us/aspnet/core/grpc/diagnostics?view=aspnetcore-5.0. There's a mix of logging, DiagnosticSource, and metrics where necessary. This only gets us running with the logging part and the others can follow up later.

Also see this great comment about logging abstractions in .NET.: aspnet/Logging#332 (comment). You can check out the entire issue -- lots of good stuff there.

Notably:

  1. Context no longer has a Logger parameter.
  2. Logger can be accessed via DI. This removes the ILogger param that we've had for WebJobs and in-proc that has caused confusion. ILogger isn't something typically injected by DI (.NET only supports ILogger<T>. It meant that in WebJobs/Functions we had to come up with a logger category that we passed to you -- which led to a bunch of other confusing issues for customers. Now you can access a logger by:
    • Injecting ILogger<T> into constructor
    • Using GetLogger extension method on context, which pulls the logger from the InstanceServices for that execution.
  3. Internally, use AsyncLocal to flag logs that are coming from us, so we know they are System logs. Note. I don't currently like this. I've commented in the PR and I'm working on a change to make this a more complete solution. Update: I've now taken care of this with the new WorkerMessage.Define. See note below
  4. Adds a logging scope with currently two properties (that we'll document like gRPC does). These provide context for any logging inside of an execution.


private static class Log
{
Copy link
Member

Choose a reason for hiding this comment

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

@brettsam brettsam merged commit 6292abb into main Feb 18, 2021
@brettsam brettsam deleted the brettsam/logging branch February 18, 2021 20:00
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.

3 participants