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

Rewrite HttpFaultInjector to use ILogger and built-in ASP.NET Core logging #7476

Merged
merged 5 commits into from
Jan 31, 2024

Conversation

lmolkova
Copy link
Member

  • Logging:
    • Leverages built-in ASP.NET Core & HttpClientFactory logging
    • Uses ILogger for a few remaining messages not covered by default logging
    • Adds OTel Console exporter to print correlated and structured logs to console
    • Configures logging to capture headers and other info captured before
  • Configuration:
    • Updates default endpoints and allows to leverage default kestrel config via env vars/cmd line args
  • Misc refactorings

Example:

LogRecord.Timestamp:               2023-12-19T21:46:21.1159635Z
LogRecord.TraceId:                 6728003b59ab450cb730ab8c5670f3f9
LogRecord.SpanId:                  5f82c7d70faf07d6
LogRecord.TraceFlags:              None
LogRecord.CategoryName:            Microsoft.AspNetCore.HttpLogging.HttpLoggingMiddleware
LogRecord.Severity:                Info
LogRecord.SeverityText:            Information
LogRecord.FormattedMessage:        Request:
Protocol: HTTP/1.1
Method: GET
Scheme: http
PathBase:
Path: /path/to/something
Accept: */*
Host: localhost:7777
User-Agent: curl/8.4.0
X-Upstream-Base-Uri: https://example.com
x-ms-faultinjector-response-option: f
LogRecord.Body:                    Request:
Protocol: HTTP/1.1
Method: GET
Scheme: http
PathBase:
Path: /path/to/something
Accept: */*
Host: localhost:7777
User-Agent: curl/8.4.0
X-Upstream-Base-Uri: https://example.com
x-ms-faultinjector-response-option: f
LogRecord.Attributes (Key:Value):
    Protocol: HTTP/1.1
    Method: GET
    Scheme: http
    PathBase:
    Path: /path/to/something
    Accept: */*
    Host: localhost:7777
    User-Agent: curl/8.4.0
    X-Upstream-Base-Uri: https://example.com
    x-ms-faultinjector-response-option: f
LogRecord.EventId:                 1
LogRecord.EventName:               RequestLog

@lmolkova lmolkova requested a review from a team as a code owner December 19, 2023 21:47
@lmolkova lmolkova changed the title Rewrite HttpFaultInjector to use ILogger and built-in logging Rewrite HttpFaultInjector to use ILogger and built-in ASP.NET Core logging Dec 19, 2023
@lmolkova lmolkova force-pushed the fault-injector-ilogger branch from 1638775 to 812ea70 Compare December 19, 2023 21:53
@mikeharder mikeharder self-requested a review January 5, 2024 01:29
@mikeharder mikeharder self-assigned this Jan 13, 2024
Copy link
Member

@mikeharder mikeharder left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you!!!

@mikeharder mikeharder assigned lmolkova and unassigned mikeharder Jan 23, 2024
@lmolkova lmolkova force-pushed the fault-injector-ilogger branch from ca726c4 to ae1cc9c Compare January 26, 2024 01:37
@lmolkova
Copy link
Member Author

@mikeharder I've rebased this change on top of streaming and changed controller to middleware (thanks for the suggestion earlier).

LMK if you want to take another too.
I've done some testing locally and going to run storage tests against this version to check it.

@mikeharder mikeharder self-requested a review January 29, 2024 20:37
@mikeharder
Copy link
Member

@lmolkova: I reviewed this again and it LGTM.

@lmolkova lmolkova enabled auto-merge (squash) January 31, 2024 00:09
@lmolkova lmolkova merged commit 2cea39b into Azure:main Jan 31, 2024
8 checks passed
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.

2 participants