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
[Fix][Plugin] sw_flask general exceptions handled #93
Conversation
tom-pytel
commented
Dec 4, 2020
•
edited
edited
- sw_flask fix will handle errors like returning the wrong type from a handler or other internal errors.
- Updated StackedSpan to track depth, and made depth variable instance instead of class level (this was a bug).
- Changed how SpanContext decides when all spans finished to write Segment data, now counts span start / stops which should work better across different async scenarios.
- Changed new_exit_span() with span.inject() to work simpler like the NodeJS agent, now plugins inject directly themselves if they need to.
- Removed carrier from plugins which didn't actually use it.
??? 😕 |
Just a random question but ... why are the meanings of span.inject() and span.extract() inverted between the Python and Node agents? Which one has it right? |
Question about |
Python has it right.
Should be called on every call to |
I understand the that this links an exit span to another service entry but the reason I ask is that multiple entry or exit spans share a single actual span which is only created once. So for example a flask entry span on top of an http server entry span is still only a single span but with a depth of 2, so does the second |
Hmm, 3.5 configuration issue? |
Might not be conf issue, suspect it’s some external changes like Pypi dependency or anything that needs to look into, will take a look |
So if you are going to remove 3.5 can you merge this anyway? |
We've set it |
I am getting strange random test failures with django 2.0, the request may succeed or it may get the span stored 2, 3 or even 4 times, but only django 2.0. Am looking into it: [EDIT] this seems to be a result of my dirty local environment since it seems to work just fine here (twice already):
|
Need to check something with you, I am thinking to change the behavior of I am checking with you first because this would change behavior (slightly, currently |
I’m +1 to this change.
No problem. Go ahead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, merging, please open another PR (if needed) for aforementioned enhancements, thanks