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

Add hint about shared context not being available in jobs handled by a daemon #9249

Closed
wants to merge 1 commit into from
Closed

Add hint about shared context not being available in jobs handled by a daemon #9249

wants to merge 1 commit into from

Conversation

Kovah
Copy link
Contributor

@Kovah Kovah commented Jan 9, 2024

Small addition to the docs to clarify the behaviour discussed in laravel/framework#48995

…a daemon

Small addition to the docs to clarify the behavior discussed in laravel/framework#48995
@taylorotwell
Copy link
Member

taylorotwell commented Jan 9, 2024

I'm not sure this is entirely true. It is available, it is just cleared per job. The point of the context is to establish context about the current request or job. For instance, you might set the log context in an HTTP or job middleware to set the current user ID so that the user ID making the current request or associated with the current job is always stored with any log messages written during that request or job.

@Kovah
Copy link
Contributor Author

Kovah commented Jan 9, 2024

For instance, you might set the log context in an HTTP or job middleware to set the current user ID so that the user ID making the current request or associated with the current job is always stored with any log messages written during that request or job.

Maybe I misunderstand the sentence. For jobs this is what I expected too, but it's not working. It's only working for regular requests.

How could the notice be rephrased so that others know that a shared log context is not available in any jobs handled by a daemon?

@rodrigopedra
Copy link

@taylorotwell, @Kovah is referring to the Log::shareContext() method, which the docs advise to be added to a Service Provider's boot() method.

Any context shared like this gets reset before the execution of each job inside the queue worker loop, by the worker's $resetScope callback.

https://github.com/laravel/framework/blob/ab9f7c4cf563be95f6e9046bce4a92801627b282/src/Illuminate/Queue/QueueServiceProvider.php#L201-L203

The Log::withContext(), which I think you might be referring to, works fine.

As the Log::withContext() method is usually called within a job middleware, or within the job itself, it acts after the queue worker scope was reset.

On issue #48995 @Kovah provided a public repo to exemplify this behavior.

IMO, adding a note to the docs, regarding this Log::shareContext() behavior on queues, is a good call.

Maybe the suggested wording right now is not clear enough, but I still think it is a good idea to clarify this behavior.

@driesvints driesvints reopened this Jan 10, 2024
@driesvints
Copy link
Member

@taylorotwell I'm just going to re-open this so you see this. I believe we should add some clarification to this section because there seems to be a lot of confusion right now on how this all works.

How would you phrase this now so it's clear what the limitations of shareContext are?

@driesvints
Copy link
Member

Btw @taylorotwell also see our Slack convo between me, @timacdonald and @themsaid

@taylorotwell
Copy link
Member

taylorotwell commented Jan 11, 2024

I've corrected the documentation example for shareContext to be correct. It should be called from within an HTTP or job middleware as it is flushed between invocation contexts. The only purpose of shareContext is to be a shortcut for calling withContext all resolved log channels in case your app uses multiple.

image

@Kovah Kovah deleted the shared-log-context branch January 12, 2024 07:55
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.

4 participants