Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add EnrichDiagnosticContextAsync to allow async calls #346

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

Open
hbunjes wants to merge 3 commits into serilog:dev
base: dev
Choose a base branch
Loading
from hbunjes:dev

Conversation

Copy link

@hbunjes hbunjes commented Sep 20, 2023

#345

If you want to add properties to the diagnostic context that require async calls (e.g. reading request body), EnrichDiagnosticContextAsync allows to hand over your own async method.

joshuajung and MoazAlkharfan reacted with thumbs up emoji
...completion
If you want to add properties to the diagnostic context that require async calls (e.g. reading request body), EnrichDiagnosticContextAsync allows to hand over your own async method.
Copy link
Member

Thanks for sending this!

I think this feature would be find if it cleanly slotted in, but as you found in the implementation, it's not completely equivalent to the existing EnrichDiagnosticContext option 🤔 - not sure how much that matters, but does suggest people might observe some unexpected differences in collected data when switching between the synchronous and asynchronous overloads.

The EnrichDiagnosticContext option we have right now also avoids adding overhead when the request completion event's level is not enabled, which is harder to do for the async version.

Just zooming out, the alternative implementation of this scenario might look like:

class RequestBodyEnrichmentMiddleware(RequestDelegate next, IDiagnosticContext diagnosticContext)
{
 public async Task InvokeAsync(HttpContext context)
 {
 await EnrichFromRequestAsync(context.Request);
 await next(context);
 }
 async Task EnrichFromRequestAsync(HttpRequest request)
 {
 // diagnosticContext.Set(...)
 }
}

Perhaps giving that example in the README would go far enough?

Copy link
Author

hbunjes commented Sep 27, 2023
edited
Loading

Nicholas, thank you for your valuable feedback. I totally get your point.

The reason I think this is still a good place to integrate is that you already have some good features like "GetLevel" in here and it's just a good match to have the logging information at one point.

I've changed the implementation to behave like the sync call. It's always called in finally block (and makes sure it doesn't create new Exceptions, just like the second call of "LogCompletion" in the exception filter clause). It's also taking the information if the method should be called based on GetLevel and current log level.

I'll check why two tests fail (it seems I've introduced a bug between my last tests and commiting the solution) and update the PR then.

The bug is fixed. In case an exception occured in one of the next middlewares or the request the logger was not initialized correctly.

{
}
finally
{
await CallEnrichDiagnosticContextAsync(httpContext, logger, level);
Copy link
Member

@nblumhardt nblumhardt Oct 3, 2023

Choose a reason for hiding this comment

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

I may be off track, but won't this be called after the LogCompletion calls in the try and catch/when blocks?

Copy link
Contributor

I would prefer to moce from public Action<IDiagnosticContext, HttpContext>? EnrichDiagnosticContext { get; set; } to public Func<IDiagnosticContext, HttpContext, Task>? EnrichDiagnosticContext { get; set; }

ivanbiljan and DMDTT reacted with thumbs up emoji

Copy link

Gonkers commented Oct 18, 2024

Is there any movement expected on this?

DMDTT, Dong-Ruipeng, and RomanKreisel reacted with eyes emoji

1 similar comment
Copy link

Is there any movement expected on this?

Dong-Ruipeng reacted with eyes emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nblumhardt nblumhardt nblumhardt left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /