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

Recursively patch any given module functions with capture #113

Merged
merged 16 commits into from
Jan 8, 2019

Conversation

hasier
Copy link
Contributor

@hasier hasier commented Nov 13, 2018

Accept a list of plain existing modules in patch that will recursively traverse all the packages inside and patch all available functions and class methods with @xray_recorder.capture().

@@ -38,6 +38,10 @@ def ready(self):
max_trace_back=settings.MAX_TRACE_BACK,
)

if settings.PATCH_MODULES:
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the late response. Other parts look good. Why capture the patching as a segment during startup? This generates dangling traces and doesn't seem to be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, thanks for your comment. I found it useful for our project, as we were patching in the settings file and the result was similar. It is also less of a hassle to add imports.
Still, I agree it does not look especially good. I gave it a thought back then and didn't come with any other solution... Any ideas? Where do you usually find useful to apply the patching in a Django app?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the right place to configure what modules to patch. I'm asking regarding the with xray_recorder.in_segment('startup'):. Do you mean you have real subsegments generated during patching so you need to create a startup segment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see. Yes, now I remember I added that because during startup I was seeing subsegments that failed to have a parent. I guess I did so because some functions decorated with .capture() are being called during startup/patching, mainly due to importing different modules (it's a really big project). I don't think it would be a big issue if we would not have a parent for those subsegments, but back then I guess I wanted to try and catch most of them. Would you do that differently?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see recursive patching can become an issue when you have some submodules get patched but they are only called during app startup. I'm OK with an additional Django configuration parameter that create a segment that wraps the startup process. But IMO this should be opt-in as some users will not want to see dangling traces that just contains startup, and this is not an issue for users that don't use this feature.

@hasier hasier force-pushed the recursive-module-patch-2 branch from 6b31f67 to 39f2ef8 Compare January 4, 2019 17:42
@haotianw465 haotianw465 merged commit edefb3d into aws:master Jan 8, 2019
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.

2 participants