Skip to content

Add support for distributed tracing when running a console command #460

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

Merged
merged 2 commits into from
Mar 29, 2021

Conversation

ste93cry
Copy link
Contributor

@ste93cry ste93cry commented Mar 6, 2021

With this PR the distributed tracing feature is activated when running a console command. If no transaction exists it starts one, otherwise it starts a span that is child of the current one. This last thing can happen if tracing has been started manually before the invocation of our listener or if the command is being executed in the context of a HTTP request where the TracingRequestListener already did its job

@ste93cry ste93cry added this to the 4.1 milestone Mar 6, 2021
@ste93cry ste93cry requested a review from Jean85 March 6, 2021 16:44
Copy link
Contributor

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 good job on thinking about the HTTP context

@stayallive
Copy link
Collaborator

Dis you take console.command from another SDK or made that up yourself?

Checking to see if we need to double check the op names were using here to make sure we keep it generally in line with expected values.

@ste93cry
Copy link
Contributor Author

ste93cry commented Mar 7, 2021

No I didn't, I don't think there is any other SDK (maybe Laravel?) that has tracing support for the console commands

@stayallive
Copy link
Collaborator

I think none have. So let's check with @HazAT what is expected and/or console commands should even be added (outside the HTTP context) at the moment (I'll ask this in Discord too to get some attention).

@ste93cry
Copy link
Contributor Author

ste93cry commented Mar 7, 2021

So let's check with @HazAT what is expected and/or console commands should even be added

We definitely want them, otherwise anything running in the context of a console command (SQL queries, etc) will not work 😅 This work is basically mirroring what has been done for starting a transaction when an HTTP request is handled

Checking to see if we need to double check the op names were using here to make sure we keep it generally in line with expected values

I think that for some things Sentry could also find a standard, for others probably not. I would not bother so much with this, for example our SDK is (I believe) the first one that is tracing SQL queries at low-level, which has a great value, and the last thing I want is to be locked with some generic name just because some other SDK introduced them before 😉

@ste93cry ste93cry force-pushed the feature/add-distributed-tracing-support-to-console branch from 8fa3c19 to 63b5eee Compare March 29, 2021 16:51
@ste93cry ste93cry merged commit 531b218 into develop Mar 29, 2021
@ste93cry ste93cry deleted the feature/add-distributed-tracing-support-to-console branch March 29, 2021 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants