Movatterモバイル変換


[0]ホーム

URL:


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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
hbunjes wants to merge3 commits intoserilog:dev
base:dev
Choose a base branch
Loading
fromhbunjes:dev

Conversation

hbunjes
Copy link

#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
…completionIf 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.
@nblumhardt
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 existingEnrichDiagnosticContext 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.

TheEnrichDiagnosticContext 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:

classRequestBodyEnrichmentMiddleware(RequestDelegatenext,IDiagnosticContextdiagnosticContext){publicasyncTaskInvokeAsync(HttpContextcontext){awaitEnrichFromRequestAsync(context.Request);awaitnext(context);}asyncTaskEnrichFromRequestAsync(HttpRequestrequest){// diagnosticContext.Set(...)}}

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

@hbunjes
Copy link
Author

hbunjes commentedSep 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

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 calledafter theLogCompletion calls in thetry andcatch/when blocks?

@sungam3r
Copy link
Contributor

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

ivanbiljan and DMDTT reacted with thumbs up emoji

@Gonkers
Copy link

Is there any movement expected on this?

DMDTT and Dong-Ruipeng reacted with eyes emoji

1 similar comment
@Dong-Ruipeng
Copy link

Is there any movement expected on this?

Dong-Ruipeng reacted with eyes emoji

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

@nblumhardtnblumhardtnblumhardt 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.

5 participants
@hbunjes@nblumhardt@sungam3r@Gonkers@Dong-Ruipeng

[8]ページ先頭

©2009-2025 Movatter.jp