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

Make tracer.start_as_current_span() decorator work with async functions #3633

Conversation

QuentinN42
Copy link
Contributor

@QuentinN42 QuentinN42 commented Jan 12, 2024

Description

Fixing the start_as_current_span decorator that report near zero time for spans of decorated async functions.

Fixes #3270

Type of change

Please delete options that are not relevant.

  • Bug fix
  • New feature
  • Breaking change : It's not a breaking change as the other libraries that implement their own Tracer class can still use the contextlib decorator (but it will still be bugged for them), as soon as they implement it, it must be working fine on their end two.
  • This change requires a documentation update

How Has This Been Tested?

  • opentelemetry-api/tests/trace/test_tracer.py : Created the minimal reproduction and fixed it.

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

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

@QuentinN42 QuentinN42 force-pushed the feat/make-tracer-start-as-current-span-decorator-work-with-async-functions branch from 11c51cb to fd9d30a Compare January 28, 2024 18:00
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Sorry for the back and forth here. I want to get @ocelotl's thoughts and consider moving back to the original approach

@QuentinN42 QuentinN42 force-pushed the feat/make-tracer-start-as-current-span-decorator-work-with-async-functions branch from 5d03fd0 to ab942de Compare February 5, 2024 19:00
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Thanks!

opentelemetry-api/src/opentelemetry/util/_decorator.py Outdated Show resolved Hide resolved
opentelemetry-api/src/opentelemetry/util/_decorator.py Outdated Show resolved Hide resolved
opentelemetry-api/tests/trace/test_proxy.py Outdated Show resolved Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

We need a comment that explains why we are using a private attribute.

@QuentinN42
Copy link
Contributor Author

  • I've added comments and tests.
  • I've also tested mypy in both version 3.8 and 3.10.

PS: I still don't really understand how tox works, sorry in advance if the pipeline is still failing :(

@aabmass
Copy link
Member

aabmass commented Feb 16, 2024

@QuentinN42 the current CI failure "metrics/integration_test/test_console_exporter.py - TypeError: 'ABCMeta... is not subscritable" I think can be solved by quoting the type annotations

@QuentinN42
Copy link
Contributor Author

I've added some code from contextlib in this commit acd649d to allow the linter to know that we are returning a Span in the with expression :

import opentelemetry.trace

TRACER = opentelemetry.trace.get_tracer(__name__)

with TRACER.start_as_current_span("foo") as span:
    print(span.get_span_context().trace_id)

@QuentinN42
Copy link
Contributor Author

I've also added comment on the tricky typing part with relevant python versions.

@QuentinN42 QuentinN42 requested a review from ocelotl February 16, 2024 22:42
@vringar vringar mentioned this pull request Feb 21, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
opentelemetry-api/tests/trace/test_tracer.py Outdated Show resolved Hide resolved
@QuentinN42
Copy link
Contributor Author

I've applied all asked changes and also merged main into my branch :)

Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
…trace api

Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
QuentinN42 and others added 18 commits March 18, 2024 16:48
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
…Manager class

Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
Signed-off-by: QuentinN42 <quentin@lieumont.fr>
@ocelotl ocelotl force-pushed the feat/make-tracer-start-as-current-span-decorator-work-with-async-functions branch from 9bcd349 to c49f8c2 Compare March 18, 2024 22:49
The missing symbol error was caused by a rebase on main and subsequent
force push by me, sorry.
@ocelotl ocelotl marked this pull request as draft March 19, 2024 00:19
@ocelotl
Copy link
Contributor

ocelotl commented Mar 19, 2024

Marking it as draft to prevent accidental merging. @QuentinN42 please check that this PR is ok, I have mistakenly force pushed changes that may have affected it, apologies for the mistake.

@QuentinN42
Copy link
Contributor Author

Just checked right now with my local branch. All seems good 👍

@QuentinN42 QuentinN42 marked this pull request as ready for review March 19, 2024 21:44
@ocelotl
Copy link
Contributor

ocelotl commented Mar 20, 2024

Just checked right now with my local branch. All seems good 👍

Thanks!

@ocelotl ocelotl merged commit 5a6da15 into open-telemetry:main Mar 20, 2024
232 checks passed
@ofekashery
Copy link

@ocelotl When can we expect a new version of opentelemetry-api with this fix to be released?

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.

Make tracer.start_as_current_span() decorator work with async functions
5 participants