fix(threading): attribute error when run is called w/o start

[dinmukhamedm]

Description

Fixes an issue where a thread run without being started would result in an attribute error.

Options considered:

Option	Pros	Cons	Chosen
Call the wrapped function directly on error	Simplest option; least intrusive	Defeats the purpose of this instrumentation; Slight risk of missing other errors	No
Only instrument run, not start	Simpler code; works for most default cases	Thread.run is designed to be overriden by custom child classes of Thread, in which case instrumentation may not work	No
Capture the context at run if not captured at start	Most precise fix for the specific breaking use case	Thread.run is designed to be overriden by custom runners, in which case instrumentation may not work	Yes

I am still open to any of the above options, and would appreciate review / further discussion.

Fixes #4245

Type of change

Please delete options that are not relevant.

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

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

• Test that the original AttributeError is removed
• Test that properly started threads propagate context

Does This PR Require a Core Repo Change?

• Yes. - Link to PR:
• No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

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

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: dinmukhamedm / name: Dinmukhamed Mailibay (0c91f20, 470ebf8, 9eff413, e47adf7, e609b0b)

[herin049]

It might be better to just move the context initialization into __init__

[dinmukhamedm]

It might be better to just move the context initialization into __init__

@herin049 I don't think that's the intended behaviour. It's common to initialize threads in one place in code and start them elsewhere. The intent is for the spawned thread to inherit the context of the place where it is start-ed, not where it is initialized.

Consider this pseudocode example

worker_threads = []
for i in range(10):
    t = threading.Thread(target=lambda: notify_downstream())
    worker_threads.append(t)

# further initialization
# ...

for t in worker_threads:
    # we want to capture this context, not the one
    # above at initialization
    with tracer.start_as_current_span(name=f"process_{i}"):
        t.start() # or t.run()

Happy to discuss further, of course.

[herin049]

It might be better to just move the context initialization into __init__

@herin049 I don't think that's the intended behaviour. It's common to initialize threads in one place in code and start them elsewhere. The intent is for the spawned thread to inherit the context of the place where it is start-ed, not where it is initialized.

Consider this pseudocode example

worker_threads = []
for i in range(10):
    t = threading.Thread(target=lambda: notify_downstream())
    worker_threads.append(t)

# further initialization
# ...

for t in worker_threads:
    # we want to capture this context, not the one
    # above at initialization
    with tracer.start_as_current_span(name=f"process_{i}"):
        t.start() # or t.run()

Happy to discuss further, of course.

Yep, you're right here, not sure what I was thinking with my original suggestion.

That being said, one potential issue that we might encounter with the current fix is if thread.run() is called directly more than once. For example:

with tracer.start_as_current_span(name="run_1"):
        t.run()
with tracer.start_as_current_span(name="run_2"):
        t.run()

With the current approach, the second call to t.run() would incorrectly have "run_1" as the parent span instead of "run_2". I doubt this will ever be an issue in practice, but the following fix does handle this scenario:

token = None
try:
    if hasattr(instance, "_otel_context"):
        token = context.attach(instance._otel_context)
    return call_wrapped(*args, **kwargs)
finally:
    if token is not None:
        context.detach(token)

Furthermore, I also don't think it really makes sense to initialize instance._otel_context to context.get_current() if thread.start() was never called. It is probably more straightforward to simply leave it uninitialized and not attach another context. Let me know what you think.

[dinmukhamedm]

Furthermore, I also don't think it really makes sense to initialize instance._otel_context to context.get_current() if thread.start() was never called. It is probably more straightforward to simply leave it uninitialized and not attach another context. Let me know what you think.

I think it still make sense to attach in run, but not store it on the instance, sort of like what you did in your example. I'll push an update later today

[herin049]

LGTM!

[xrmx]

@dinmukhamedm Please fix the conflicts, thanks

[dinmukhamedm]

Please fix the conflicts, thanks

done

[xrmx]

@dinmukhamedm please pull main. Opening PRs from company repositories where we cannot push on our own is painful since for merging we require the branch to include latest main.

[dinmukhamedm]

@xrmx updated and invited you as an outside collaborator to the fork, just in case