Skip to content

Refactor: moved exception check to Span.__exit__#83

Merged
kezhenxu94 merged 1 commit intoapache:masterfrom
tom-pytel:master
Nov 19, 2020
Merged

Refactor: moved exception check to Span.__exit__#83
kezhenxu94 merged 1 commit intoapache:masterfrom
tom-pytel:master

Conversation

@tom-pytel
Copy link
Copy Markdown
Contributor

Moved exception check for spans into Span.exit from multiple individual locations in individual plugins. This should also catch other errors that may not be recognized otherwise. Is there any reason not to do it like this?

Copy link
Copy Markdown
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

Hey @tom-pytel, thanks very much for noticing this, I have only one question inline

Comment thread skywalking/trace/span.py
@kezhenxu94 kezhenxu94 added core enhancement New feature or request labels Nov 19, 2020
@kezhenxu94 kezhenxu94 added this to the 0.5.0 milestone Nov 19, 2020
Copy link
Copy Markdown
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

@tom-pytel could you please kindly refactor the counterpart in context.new_entry_span as well? Now you just refactored the context.new_exit_span, really appreciate 🙇

e.g.

try:
__on_deliver(this, method_frame, header_frame, body)
span.tag(Tag(key=tags.MqBroker, val=peer))
span.tag(Tag(key=tags.MqTopic, val=exchange))
span.tag(Tag(key=tags.MqQueue, val=routing_key))
except BaseException as e:
span.raised()
raise e

@tom-pytel
Copy link
Copy Markdown
Contributor Author

@tom-pytel could you please kindly refactor the counterpart in context.new_entry_span as well? Now you just refactored the context.new_exit_span, really appreciate 🙇

e.g.

try:
__on_deliver(this, method_frame, header_frame, body)
span.tag(Tag(key=tags.MqBroker, val=peer))
span.tag(Tag(key=tags.MqTopic, val=exchange))
span.tag(Tag(key=tags.MqQueue, val=routing_key))
except BaseException as e:
span.raised()
raise e

The exception checking is in class Span which is base for EntrySpan, ExitSpan and NoopSpan so it should work for all.

@kezhenxu94
Copy link
Copy Markdown
Member

The exception checking is in class Span which is base for EntrySpan, ExitSpan and NoopSpan so it should work for all.

I meant removing the try-catch in the new_entry_span block, I missed that you already removed them 😄 thanks

Copy link
Copy Markdown
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

Thanks

@kezhenxu94 kezhenxu94 merged commit b101320 into apache:master Nov 19, 2020
@tom-pytel
Copy link
Copy Markdown
Contributor Author

Thanks

No problem 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants