Skip to content

[Hosting] Add {OriginalFormat} to start & stop log state objects #45253

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 2 commits into from
Dec 7, 2022

Conversation

CodeBlanch
Copy link
Contributor

Description

The ILogger extensions and source generator return the original template logged as the {OriginalFormat} key on the IReadOnlyList<KeyValuePair<string, object>> state representing the log being written.

AspNetCore has a couple custom log states which do not mirror the {OriginalFormat} behavior. By default OpenTelemetry only captures message templates but for these logs it must either have empty template/body (confusing for users) or invoke ToString (and incur allocations) to generate a formatted string because no template is available.

In order to make these high value logs play nicely with structured logging and to enable high performance capture I'm proposing we should mirror the {OriginalFormat} behavior.

Changes

  • Returns {OrignalFormat} as the last key on HostingRequestStartingLog & HostingRequestFinishedLog state objects
  • Tweaks the log messages to be more consistent & deterministic...
    • Start log now writes content type & length after a "-" character
    • Stop log no longer writes out request content type & length

Final templates look like this:

Start:
Request starting {Protocol} {Method} {Scheme}://{Host}{PathBase}{Path}{QueryString} - {ContentType} {ContentLength}

Stop:
Request finished {Protocol} {Method} {Scheme}://{Host}{PathBase}{Path}{QueryString} - {StatusCode} {ContentLength} {ContentType} {ElapsedMilliseconds}ms

/cc @davidfowl @tarekgh @noahfalk @cijothomas @utpilla @reyang

@ghost ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels Nov 23, 2022
@ghost
Copy link

ghost commented Nov 23, 2022

Thanks for your PR, @CodeBlanch. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@BrennanConroy
Copy link
Member

If we're going to do this, we should do it for all our custom logging:
https://grep.app/search?q=IReadOnlyList%3CKeyValuePair%3Cstring%2C%20object%3F%3E%3E&filter[repo][0]=dotnet/aspnetcore

@CodeBlanch
Copy link
Contributor Author

@BrennanConroy Thanks for the link! I don't think {OriginalFormat} is really needed on scopes. HttpRequestLog & HttpResponseLog probably need it, but not sure what the template would be exactly. In any case, happy to look into those things if this goes in 👍

@adityamandaleeka
Copy link
Member

cc @samsp-msft

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

Agreed about the scopes. Even for the HttpLogging middleware, I don't think there's a great {OriginalFormat} since it's so dynamic. I think we should just take this as a starting point. If someone else has a good idea for the other logs, we can merge that separately.

@adityamandaleeka adityamandaleeka merged commit dd50702 into dotnet:main Dec 7, 2022
@ghost ghost added this to the 8.0-preview1 milestone Dec 7, 2022
@CodeBlanch CodeBlanch deleted the hosting-log-originalformat branch December 8, 2022 18:19
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants