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

Recreate span on every run of a decorated function #1451

Merged

Conversation

anton-ryzhov
Copy link
Contributor

@anton-ryzhov anton-ryzhov commented Dec 7, 2020

Description

start_as_current_span could be used as decorator. But it works incorrectly in this case — a span is created only once on decoration time and it is reused for every run of function.

With that change it creates new span on every run.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Unit test provided

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@anton-ryzhov anton-ryzhov marked this pull request as ready for review December 7, 2020 10:07
@anton-ryzhov anton-ryzhov requested a review from a team as a code owner December 7, 2020 10:07
@anton-ryzhov anton-ryzhov requested review from toumorokoshi and aabmass and removed request for a team December 7, 2020 10:07
@anton-ryzhov
Copy link
Contributor Author

Any comments?

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.

This is a really interesting use case! I see the value of being able to cheaply wrap functions like this.

My one concern is the function isn't intuitively named to indicate it's a decorator. It may be good to add examples to the docs/faq page.

@anton-ryzhov
Copy link
Contributor Author

I thought it was implied to use it as a decorator, that's rather expected to me. Saves one indentation level.

named to indicate it's a decorator.

How decorators should be named? Is there any special convention on it? I don't know any.

@ocelotl
Copy link
Contributor

ocelotl commented Dec 11, 2020

I thought it was implied to use it as a decorator, that's rather expected to me. Saves one indentation level.

named to indicate it's a decorator.

How decorators should be named? Is there any special convention on it? I don't know any.

There are no naming conventions for decorators. Mentioning in the documentation that this function can be used also as a decorator would be valuable, since every example so far does not use this pattern.

@@ -760,7 +761,8 @@ def start_as_current_span(
record_exception=record_exception,
set_status_on_exception=set_status_on_exception,
)
return self.use_span(span, end_on_exit=True)
with self.use_span(span, end_on_exit=True) as span_context:
yield span_context

def start_span( # pylint: disable=too-many-locals
Copy link
Member

Choose a reason for hiding this comment

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

Does this same decorator pattern work with start_span()? I would expect them to behave the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't use @contextmanager, so no. And it should not

@owais
Copy link
Contributor

owais commented Dec 11, 2020

This is a really interesting use case! I see the value of being able to cheaply wrap functions like this.

My one concern is the function isn't intuitively named to indicate it's a decorator. It may be good to add examples to the docs/faq page.

Agree with this. A better name would be nice. Ignoring all other Otel conventions, something like @otel.trace(name='') would have been most intuitive name for me but we probably want to stay closer to otel conventions and almost everywhere else we use span in functions that start or create spans. So, perhaps something like @with_span(...) or @record_span(...)?

@anton-ryzhov
Copy link
Contributor Author

anton-ryzhov commented Dec 14, 2020

Some pipelines are broken after branch update. I don't know why but pretty sure not because of this PR.

Naming question — is it part of this PR? Could you please merge that bugfix first?

@anton-ryzhov
Copy link
Contributor Author

Lets merge it before CI will disapprove it again?

@codeboten
Copy link
Contributor

@anton-ryzhov looks like we missed it again if you can update the branch

@anton-ryzhov
Copy link
Contributor Author

@codeboten updated

@anton-ryzhov
Copy link
Contributor Author

Any chance that this will be merged?

With current setup CHANGELOG file will always have conflicts, so only chance to merge PR is to update it right before merging. But the PR is missed again and again.

@codeboten
Copy link
Contributor

codeboten commented Jan 4, 2021

@anton-ryzhov if you can resolve the conflict, @lzchen and I can hold off on merging any other changes until this one passes CI and gets merged

@anton-ryzhov
Copy link
Contributor Author

Updated

@codeboten codeboten merged commit 336af87 into open-telemetry:master Jan 4, 2021
@codeboten
Copy link
Contributor

Thanks @anton-ryzhov! Merged

@anton-ryzhov
Copy link
Contributor Author

Thank you

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.

None yet

6 participants