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

Fix handling of slog logs before logging initialized #707

Merged
merged 14 commits into from
May 1, 2024
Merged

Conversation

mattdurham
Copy link
Collaborator

@mattdurham mattdurham commented Apr 29, 2024

PR Description

Previously if you tried to use the logger for a slog style logger the system would panic. This adds a deferred handler for handling slog entries. The log buffer now supports normal logs and slog.Records and replays them in the appropriate order.

This is a lot of code to handle replaying of logs, its possible I could merge the deferred handler and the handler but then I think the logic would get real complicated.

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated

@mattdurham mattdurham marked this pull request as ready for review April 29, 2024 20:21
@mattdurham mattdurham requested review from rfratto and wildum April 29, 2024 20:21
@mattdurham mattdurham changed the title Fix slog Fix handling of slog logs before logging initialized Apr 29, 2024
@mattdurham mattdurham requested review from erikbaranowski and removed request for wildum April 30, 2024 12:46
@mattdurham mattdurham requested a review from rfratto May 1, 2024 12:57
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

Thanks! This is my last round of feedback, should be good to go after this

internal/alloy/logging/deferred_handler.go Outdated Show resolved Hide resolved
internal/alloy/logging/slogtest.go Outdated Show resolved Hide resolved
internal/alloy/logging/logger.go Outdated Show resolved Hide resolved
internal/alloy/logging/logger.go Outdated Show resolved Hide resolved
internal/alloy/logging/logger.go Outdated Show resolved Hide resolved
internal/alloy/logging/handler.go Outdated Show resolved Hide resolved
internal/alloy/logging/handler.go Outdated Show resolved Hide resolved
internal/alloy/logging/handler.go Show resolved Hide resolved
internal/alloy/logging/logger.go Outdated Show resolved Hide resolved
internal/alloy/logging/deferred_handler.go Outdated Show resolved Hide resolved
@mattdurham mattdurham requested a review from rfratto May 1, 2024 18:54
@mattdurham
Copy link
Collaborator Author

Made the changes, also good for pushing to use the default slogtest. I have to add a little hack but it works and is much cleaner.

Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

LGTM!

internal/alloy/logging/logger.go Outdated Show resolved Hide resolved
@mattdurham mattdurham enabled auto-merge (squash) May 1, 2024 19:31
@mattdurham mattdurham merged commit a7f11dd into main May 1, 2024
13 checks passed
@mattdurham mattdurham deleted the fix_slog branch May 1, 2024 19:36
polyrain pushed a commit to polyrain/alloy that referenced this pull request May 2, 2024
* add handlers

* Add support for handling slog style logging that happens BEFORE the logger is initialized.

* Update changelog

* unexport deferred handler

* clean up and add tests

* refactor tests

* fix refactor misname

* fix test

* cleanup changes

* Use slog tests.

* Remove DeferredHandler from Logger and always use a wrapped deferred handler.

* pr feedback

* Add more support for logging
@rfratto rfratto added the backport-to-agent:no PR should NOT be backported to the agent repo. label May 3, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport-to-agent:no PR should NOT be backported to the agent repo. frozen-due-to-age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants