Instrumentation for Pyramid #776
Conversation
cnnradams
force-pushed
the
pyramid_instrumentation
branch
3 times, most recently
from
340e29b
to
d106094
Compare
| settings = kwargs.get("settings", {}) | ||
| tweens = settings.get("pyramid.tweens") | ||
|
|
||
| if tweens and tweens.strip() and TWEEN_NAME not in settings: |
There was a problem hiding this comment.
Is settings dict guaranteed to return strings? Might be worth it adding a check for that before calling .strip() if not.
There was a problem hiding this comment.
updated it to follow closely what pyramid does with its tween list. Also made it so the middleware is always at the start vs sometimes before EXCVIEW, and sometimes at the end, which is what was currently there.
|
|
||
| def traced_init(wrapped, instance, args, kwargs): | ||
| settings = kwargs.get("settings", {}) | ||
| tweens = settings.get("pyramid.tweens") |
There was a problem hiding this comment.
Nit: If pyramid.tweens is guaranteed to be a string, this could be changed to simplify the check below.
tweens = settings.get("pyramid.tweens", "").strip()There was a problem hiding this comment.
updated, see above comment
| settings = registry.settings | ||
| enabled = asbool(settings.get(SETTING_TRACE_ENABLED, True)) | ||
|
|
||
| if enabled: |
There was a problem hiding this comment.
nit:
if not enabled:
return
...
| # that's still valuable information that isn't necessarily | ||
| # a 500. For instance, HTTPFound is a 302. | ||
| # As described in docs, Pyramid exceptions are all valid | ||
| # response types |
There was a problem hiding this comment.
Does this actually modify the handler behavior? Would disabling the instrumentation result in an exception being raised while enabling it would swallow the exception and return it as a value instead? Not sure if an instrumentation should modify behavior like that. I think we should preserve behavior and just record what happened in spans.
There was a problem hiding this comment.
I think in both cases the exception would be raised - we raise at the end of the except, which calls the finally block to record to the span and then the exception is reraised
There was a problem hiding this comment.
I probably don't understand try..except..finally with raise + return at all but I ran a small test and it seems the exception is not re-raised if the finally block contains a return statement.
Also, if the intention is for the exception to be re-raised, I'd suggest to assign the exception to something other than the response to make it the intention clear.
There was a problem hiding this comment.
https://repl.it/@OwaisLone1/ElectricImperturbableListener#main.py test but may be I'm doing something wrong?
There was a problem hiding this comment.
right, if you have a return in the finally it clobbers the exception. However we don't here, the return is outside the finally block (I can definitely see how its hard to tell, I should add a new line before the return). I can see what you mean by not assigning the exception to the response, but it lets us re-use the code in the finally block since the HTTPException has all of the properties necessary to fill out the span. Up for suggestions if you can think of a better way.
There was a problem hiding this comment.
How about assigning both response from the try block and the exception to a 3rd var name result or response_or_exception (or something else) and using that to fill out the span? That would make it very clear enough I think.
There was a problem hiding this comment.
changed to response_or_exception, let me know if it looks better now (I think it does)
There was a problem hiding this comment.
Yes, more obvious for sure. I actually meant we catch response in the response variable but also assign it to response_or_exception. That way response var will be returned and response_or_exception will be used to extract metadata. Just a suggestion :)
There was a problem hiding this comment.
That makes a lot more sense :P updated to return response and get span attributes from response_or_exception
cnnradams
force-pushed
the
pyramid_instrumentation
branch
2 times, most recently
from
89fc53c
to
0187389
Compare
| instance.include("opentelemetry.ext.pyramid.callbacks") | ||
|
|
||
|
|
||
| def unwrap(obj, attr: str): |
There was a problem hiding this comment.
I believe this now exists in the utility library you added :)
|
|
||
| def trace_tween_factory(handler, registry): | ||
| settings = registry.settings | ||
| enabled = asbool(settings.get(SETTING_TRACE_ENABLED, True)) |
There was a problem hiding this comment.
As I understand this function, if tracing is disabled during the initialization for the application, then we will never instrument the application at any point for its runtime, correct? as the trace_tween_factory is only called once during initialization, and it in the case where the trace is disabled, it'll return the disabled tween.
IOW, modifying the settings afterward has no impact on what is being traced.
There was a problem hiding this comment.
I haven't actually verified this, but I'm pretty sure the tween factory is called whenever make_wsgi_app is called. so if we uninstrument_config() which sets enabled = False, and then export the wsgi app, enabled would end up being False. If this wasn't the case I'm not sure how this test would work, since we never removed the tween
cnnradams
force-pushed
the
pyramid_instrumentation
branch
from
0187389
to
33d9387
Compare
Co-authored-by: Yusuke Tsutsumi <yusuke@tsutsumi.io>
Co-authored-by: Yusuke Tsutsumi <yusuke@tsutsumi.io>
This instrumentation follows the same pattern as the Flask implementation, to be able to create the span with the proper name (different than ddtrace/opencensus implementations)
Resolves #632