-
Notifications
You must be signed in to change notification settings - Fork 11.5k
[12.x] Introduce ContextLogProcessor
#54851
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
Conversation
Thanks for submitting a PR! Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface. Pull requests that are abandoned in draft may be closed due to inactivity. |
@timacdonald I still want to add tests for this, but would be curious to get your thoughts on which approach makes the most sense. edit: I thought about this more, and I think it makes sense to just create a ProcessorInterface in the Context namespace and bind it via the ContextServiceProvider. User land can override it, and there's no need to clutter the LogManager. edit 2: Added the changes and updated the description. |
LogManager::setContextToLogProcessor()
LogManager::setAddContextToLogProcessor()
LogManager::setAddContextToLogProcessor()
ContextLogProcessor
This seems reasonable to me, is not introducing complexity, and creates some additional and interesting flexibility for Context. I like it! My only casual thought is we may want to introduce a dedicated |
Good call. 👍 I agree that feels much better. |
Why
There are two particular things I would like to be able to do with how context is used with logs.
extra
, but instead, want it merged into the existing logcontext
. This makes querying DataDog easier, since it doesn't require me to know if it's a value set in Context versus added viaLog::withContext()
or added to the log context viaLog::info('some important log', ['a-value-i-care-about' => true])
.Number two is solvable by storing the log channel in the Context's hidden data and pushing an additional processor to the set the log channel based on that.
What is proposed in this PR is that we move the context log processor into a separate class. I can achieve both of my goals with minimal impact to the framework's code. Now in userland, I can easily override the bound processor class. For 99.99% of users, they won't ever use this. For those who want control over how context is merged into the log record, they have an idiomatic way of configuring that.
An example of how I might use this
Other Possible Approaches
setContextToLogProcessor
on either the LogManager or the Context\Repository. This was the initial approach I took, but it seemed kinda wonky/unwieldy for something so few people might ever use. Binding a concrete class and allowing override makes this much more seamless.Log::popProcessor()
,Log::pushProcessor()
themselves. It's workable, but might not be reliable, since it's possible another processor got pushed by a third-party packageextra
but.... eh.... I find that to be annoying.extra
orcontext
on theMonolog\LogRecord
. This feels considerably less robust.Also...
While playing around with this, I realized you cannot add processors to the
single
driver via the config. Wouldn't solve the problem, but it seems weird that it doesn't allow for this.