opentelemetry-python icon indicating copy to clipboard operation
opentelemetry-python copied to clipboard

TypeError from opentelemetry.context.contextvars_context in detach

Open hyoinandout opened this issue 1 year ago • 5 comments

Describe your environment

Python version: 3.12.4 SDK version: 1.25.0 API version: 1.25.0

What happened?

TypeError from opentelemetry.context.contextvars_context in detach. The function detach expected an instance of Token, but it got None. Thus, it fails to detach context.

Steps to Reproduce

Pass None to _RUNTIME_CONTEXT.detach

Expected Result

This kind of error should be elaborated, without avoiding mypy

Actual Result

TypeError occurs

Additional context

No response

Would you like to implement a fix?

Yes

hyoinandout avatar Aug 29 '24 06:08 hyoinandout

Why do you think this should handled instead of delegating to mypy?

xrmx avatar Aug 30 '24 15:08 xrmx

Hi, afaik, the related line here is ignoring mypy. In opentelemetry-api/src/opentelemetry/context/contextvars_context.py#50

self._current_context.reset(token)  # type: ignore

Due to this comment, mypy cannot detect TypeError.

If you are suggesting to uncomment the ignore, I'm ok, the draft PR is just my initial proposal.

hyoinandout avatar Sep 02 '24 04:09 hyoinandout

Hi, afaik, the related line here is ignoring mypy. In opentelemetry-api/src/opentelemetry/context/contextvars_context.py#50

self._current_context.reset(token)  # type: ignore

Due to this comment, mypy cannot detect TypeError.

If you are suggesting to uncomment the ignore, I'm ok, the draft PR is just my initial proposal.

Missed that, thanks. Yeah, I think it would make sense to add type annotations here if feasible

xrmx avatar Sep 02 '24 08:09 xrmx

I added type annotations - could you have a check on the latest PR of this issue?

hyoinandout avatar Sep 04 '24 06:09 hyoinandout

@xrmx I managed to reproduce a problem with a 'None' token using a partially instrumented celery (sender not instrumented, worker instrumented) as seen above. This causes the context.detach to fail. I'm not sure if the fix is simply to to check if the token is not None before calling detach ?

rslinckx avatar Sep 04 '24 19:09 rslinckx

Looks like this was fixed in https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2927

alexmojaki avatar Dec 16 '24 15:12 alexmojaki

I think we (thanks to all who helped in getting this merged!) made the type annotations here clearer in https://github.com/open-telemetry/opentelemetry-python/pull/4346 so this can be caught using mypy etc. now.

mrnicegyu11 avatar Mar 25 '25 17:03 mrnicegyu11

Right, closing this since annotations are in. Thanks!

xrmx avatar Mar 26 '25 15:03 xrmx