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

Add is_recording for exception recording #1386

Merged
merged 2 commits into from
Nov 24, 2020
Merged

Conversation

lzchen
Copy link
Contributor

@lzchen lzchen commented Nov 17, 2020

Forgot to do this as part of #1365.

Should be merged after open-telemetry/opentelemetry-python-contrib#190 since requests instrumentation uses the constant.

@lzchen lzchen requested review from a team, toumorokoshi and hectorhdzg and removed request for a team November 17, 2020 18:40
@toumorokoshi
Copy link
Member

toumorokoshi commented Nov 18, 2020

If I understand the general motivation of #1301, it's to eliminate the duplication in span ending logic that exists in both Tracer.use_span as well as the span context manager itself. I don't think this PR accomplishes that goal.

I actually think this could be achieved by wrapping this yield statement in the contextmanager exposed by the span:

with span as span:
    yield span   # exception handling is now handled by Span.__exit__

And deleting the now-redundant code in use_span.

What do you think?

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

have a question clarifying the intent of the change, and proposing a different way to achieve the result.

@lzchen
Copy link
Contributor Author

lzchen commented Nov 18, 2020

@toumorokoshi

I think I linked this issue out of context. You are exactly right, the PR that was responsible for fixing #1301 was already merged here. I just forgot to do the is_recording() check as part of that PR.

As for your suggestions, @aabmass actually suggested the exact same solution, which I answered here. Sorry if the description was misleading.

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification! I'll approve now, although I do think there is merit in looking into the refactoring of code.

@lzchen lzchen merged commit c1de387 into open-telemetry:master Nov 24, 2020
@lzchen lzchen deleted the exc branch November 24, 2020 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants