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

Refactor frontend logging #926

Merged
merged 3 commits into from
Jan 15, 2025
Merged

Refactor frontend logging #926

merged 3 commits into from
Jan 15, 2025

Conversation

mbarnes
Copy link
Collaborator

@mbarnes mbarnes commented Dec 4, 2024

What this PR does

I wanted to make what I thought was a small change to the frontend logs (essentially the last commit here). But when I realized the logger attribute I added ("api_version") was not showing up in logs, the ensuing investigation led me down a rabbit hole that resulted in some major logging refactoring.

See the individual commit messages for more context.

@mbarnes mbarnes force-pushed the frontend-logging branch 2 times, most recently from dc243b6 to b9ff0c4 Compare December 4, 2024 17:02
@mbarnes mbarnes marked this pull request as ready for review December 6, 2024 13:48
@mbarnes mbarnes enabled auto-merge December 6, 2024 17:00
Copy link

Please rebase pull request.

Copy link

github-actions bot commented Jan 7, 2025

Please rebase pull request.

Copy link

github-actions bot commented Jan 8, 2025

Please rebase pull request.

mbarnes pushed a commit that referenced this pull request Jan 14, 2025
Extracted from #926 to
avoid conflicts. This commit will go away once 926 is merged.
Copy link

Please rebase pull request.

mbarnes pushed a commit that referenced this pull request Jan 15, 2025
Extracted from #926 to
avoid conflicts. This commit will go away once 926 is merged.
Matthew Barnes added 3 commits January 15, 2025 08:37
LoggerFromContext will never fail now.

Middleware functions were falling back to the default logger, but
we do this from within LoggerFromContext now so the fallback logic
doesn't have to be repeated everywhere.
Always retrieve the logger from the request context. This ensures
we're using the correct logger in case middleware functions add a
logging attribute.
Add a logger attribute for the request's "api-version" parameter
from MiddlewareValidateAPIVersion.

Also remove the "<api-version>: <function-name>" log messages from
the HTTP handler methods. I added these messages when the handlers
were no more than stubs to test request routing but they no longer
add value to the frontend logs.
Copy link
Collaborator

@mociarain mociarain left a comment

Choose a reason for hiding this comment

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

Nice!

@mbarnes mbarnes merged commit 2cf3f00 into main Jan 15, 2025
10 checks passed
@mbarnes mbarnes deleted the frontend-logging branch January 15, 2025 13:57
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