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

[API Proposal]: Simpler, neater and more memorable contextual logging #110646

Closed
lonix1 opened this issue Dec 12, 2024 · 9 comments
Closed

[API Proposal]: Simpler, neater and more memorable contextual logging #110646

lonix1 opened this issue Dec 12, 2024 · 9 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Logging
Milestone

Comments

@lonix1
Copy link

lonix1 commented Dec 12, 2024

Background and motivation

One can use ILogger to enrich log events with extra data:

using (_logger.BeginScope(new Dictionary<string, object?>
  {
    { "Foo", "abc" },
    { "Bar", 123 },
    { "Baz", true }
})) {
  _logger.LogInformation("Process returned {Result}", 42);
  _logger.LogInformation("Process returned {Result}", 43);
}

That is horrible, and hard to remember. That syntax must be used even to enrich a single log event.

For a single log event, Serilog's syntax is a pleasure:

_logger
  .ForContext("Foo", "abc")
  .ForContext("Bar", 123)
  .ForContext("Baz", true)
  .LogInformation("Process returned {Result}", 42);

...And that makes it easy to remember and use.

API Proposal

Unsure.

Maybe:

public static ILogger Enrich(this ILogger logger, string key, object? value);

Or:

public static ILogger Enrich(this ILogger logger, params (string, object?)[] properties);

However those contextual properties should only apply to the next log event. So like I said, I'm unsure what the signatures should look like. Maybe something new is required.

API Usage

A compromise between MEL and Serilog syntax:

For a single log event:

_logger
  .Enrich("Foo", "abc")
  .Enrich("Bar", 123)
  .Enrich("Baz", true)
  .LogInformation("Process returned {Result}", 42);

For multiple log events: either the same as we have now, or:

using (_logger.Enrich(
    ("Foo", "abc"),
    ("Bar", 123),
    ("Baz", true)
  )) {
  _logger.LogInformation("Process returned {Result}", 42);
  _logger.LogInformation("Process returned {Result}", 43);
}

Alternative Designs

I posted some workarounds on StackOverflow, which are much nicer. But they're still not as good as what could be done within the runtime. Some syntactic sugar would be great.

Risks

No response

@lonix1 lonix1 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Dec 12, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Dec 12, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-extensions-logging
See info in area-owners.md if you want to be subscribed.

@julealgon
Copy link

I feel like leveraging the new enrichment extension point from Microsoft.Extensions.Telemetry solves most of the issues I personally had with this pattern, as a ton of that stuff is contextual data that can be added with one or many enricher implementations the same way that it can be done with Serilog.

For the few cases where I need to add inline data, it would be nice to have a simpler syntax though, so I'm not opposed to this proposal.

@tarekgh
Copy link
Member

tarekgh commented Dec 12, 2024

@geeknoid @evgenyfedorov2 @joperezr Could you look into this and determine if it can be implemented using the Enrichment feature in the extensions? Is there a way to achieve this with the current model? Or you consider adding this feature there?

@tarekgh tarekgh added this to the Future milestone Dec 12, 2024
@tarekgh tarekgh removed the untriaged New issue has not been triaged by the area owner label Dec 12, 2024
@geeknoid
Copy link
Member

The enrichment model we provide is considerably more powerful and general purpose in nature. There is a chain of enrichers which can supplement log records with either static information (like cluster id for example) or with dynamic per-request state. It's a different logical model than what is proposed here however.

@tarekgh
Copy link
Member

tarekgh commented Dec 12, 2024

Thanks @geeknoid, does it make sense to consider the proposal here for potential support?

@geeknoid
Copy link
Member

I don't think the proposal provides enough bang-for-the-buck, given the existence of the bigger enrichment story.

@tarekgh
Copy link
Member

tarekgh commented Dec 12, 2024

Thanks, @lonix1, for your suggestions. After discussion, we have decided not to pursue this proposal at this time. However, we will monitor for increased demand for similar proposals and may reconsider our decision in the future.

@tarekgh tarekgh closed this as not planned Won't fix, can't repro, duplicate, stale Dec 12, 2024
@julealgon
Copy link

@geeknoid so for very localized, non-contextual additional data, you suggest log scopes with dictionaries to be the go-to way to do those still? Would that be correct?

I ask because not all types of data can be injected using a singleton or a scoped enricher. Sometimes, there is data that is very local to a deeply nested method in the call graph that you still want to instrument in a block of child logger calls.

I do use log scopes for that in a similar way than what was presented here but was wondering if you would recommend a different strategy.

@geeknoid
Copy link
Member

@julealgon My perspective on scopes is that they are a somewhat orphaned feature, and if logging was to be reimplemented in >NET, this feature probably wouldn't exist.

Across Microsoft, I've seen very very little use of scopes in real production contexts. Lexical scoping is often not the right model, especially in the face of async work. Most scenarios simply don't need this, or you end up lugging around your own bag of state for those specialized cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Logging
Projects
None yet
Development

No branches or pull requests

4 participants