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

feat(profiling): Set active thread id for quart #1830

Merged
merged 1 commit into from Mar 6, 2023

Conversation

Zylphrex
Copy link
Member

Following up to #1824 to set the active thread id for quart.

@sl0thentr0py
Copy link
Member

@pgjones we're making some changes to the Quart integration to enable our (beta) profiler on ASGI frameworks, in case you want to take a quick look at the changes!


with hub.configure_scope() as sentry_scope:
if sentry_scope.profile is not None:
sentry_scope.profile.active_thread_id = (
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the active_thread_id used for? Quart doesn't really use threads, so I'm interested to learn what the need is.

Copy link
Member Author

Choose a reason for hiding this comment

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

For context, we're working on adding profiling support for the various ASGI frameworks, and when profiling, we have the concept of an active profile which for the purpose of a web server, it is the thread on which the request is handled.

In this case, we're specifically targeting synchronous request handlers because the way they're handled is by dispatching the handler to another thread. So what we're trying to do here is after it's been dispatched, we want to determine the thread id the handler is running on and update the profile metadata to be aware of that.


def patch_scaffold_route():
# type: () -> None
old_route = Scaffold.route
Copy link
Contributor

Choose a reason for hiding this comment

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

route isn't the only way to create a handler, there is also add_url_rule and the corresponding ones for websockets.

Did you consider using a before_request function as a place to run some code for every request?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at before_request, I'm not sure it'll serve the use case we want here because it we specifically want to read the thread id of the thread running the request handler and from what I can see, before_request is called from the main thread and potentially dispatched to another thread independently from the request handler.

@pgjones
Copy link
Contributor

pgjones commented Jan 14, 2023

I understand. Had you considered patching the app.ensure_async method instead? This would give the correct thread ID in the context for other functions bar route handlers.

@Zylphrex
Copy link
Member Author

I understand. Had you considered patching the app.ensure_async method instead? This would give the correct thread ID in the context for other functions bar route handlers.

I did consider that at one point but I noticed that ensure_async is called by more than just the route handlers. And if I understood it correctly, that means after setting the thread ID for the handler, other methods can happen and change the thread ID. Is this correct? If so, I don't think patching ensure_async is the right thing to do here.

@pgjones
Copy link
Contributor

pgjones commented Jan 16, 2023

Yes, I think that is correct. However, the other methods could be a before/after function which I think would also ideally be recorded in the transaction? (As the total time to respond will include these). If not I think what is in this PR does make the most sense.

@Zylphrex
Copy link
Member Author

Yes, I think that is correct. However, the other methods could be a before/after function which I think would also ideally be recorded in the transaction? (As the total time to respond will include these). If not I think what is in this PR does make the most sense.

So even if the other calls to ensure_async are also captured in the same transaction, I think the justification here is that these other calls are not the "core" work done for that transaction. So if one of these calls happens after the request handler, we'd end up showing a different thread as the active thread for the profile which can be misleading to the end user. So while this approach will only correctly show the right thread for some requests, I think it's better the result is predictable rather than relying on how other code calls ensure_async to not change the active thread id.

Base automatically changed from txiao/feat/enable-profiling-for-asgi-frameworks to master January 17, 2023 20:49
@Zylphrex Zylphrex force-pushed the txiao/feat/set-active-thread-id-for-quart branch from 03ff178 to 073e658 Compare January 19, 2023 19:57
@github-actions
Copy link

github-actions bot commented Mar 4, 2023

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

Following up to #1824 to set the active thread id for quart.
@sl0thentr0py sl0thentr0py force-pushed the txiao/feat/set-active-thread-id-for-quart branch from 896fcb7 to 1f60e7b Compare March 6, 2023 13:10
@sl0thentr0py
Copy link
Member

sry this flew under the radar

@sl0thentr0py sl0thentr0py merged commit dad343e into master Mar 6, 2023
228 checks passed
@sl0thentr0py sl0thentr0py deleted the txiao/feat/set-active-thread-id-for-quart branch March 6, 2023 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants