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

Fix issue with Flask instrumentation when a request spawn children threads and copies the request context #1654

Merged

Conversation

hangonlyra
Copy link
Contributor

@hangonlyra hangonlyra commented Feb 9, 2023

Description

In opentelemetry.instrumentation.flask record the thread ID in _before_request in addition to the span. Then in _teardown_request check that it's the same thread for that span before calling activation.__exit__

Fixes #1653
Fixes #1551

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

The bug is reproducible with this simple Flask app:

from concurrent.futures import ThreadPoolExecutor, as_completed
from random import randint

from flask import Flask, copy_current_request_context

from opentelemetry.instrumentation.flask import FlaskInstrumentor

app = Flask(__name__)

def do_random_stuff():
    @copy_current_request_context
    def inner():
        return randint(0, 100)
    return inner

@app.route("/")
def hello_world():
    count = 4
    executor = ThreadPoolExecutor(count)
    futures = []
    for _ in range(count):
        futures.append(executor.submit(do_random_stuff()))

    for future in as_completed(futures):
        print(future.result())

    return "Hello, World!"

FlaskInstrumentor.instrument_app(app)

When run with opentelemetry-instrument --traces_exporter console --metrics_exporter console flask run and accessed using HTTP it results in

    self.app.do_teardown_request(exc)
  File "~/otelbug/env/lib/python3.9/site-packages/flask/app.py", line 2373, in do_teardown_request
    self.ensure_sync(func)(exc)
  File "~/otelbug/env/lib/python3.9/site-packages/opentelemetry/instrumentation/flask/__init__.py", line 441, in _teardown_request
    activation.__exit__(None, None, None)
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/contextlib.py", line 126, in __exit__
    next(self.gen)
ValueError: generator already executing

After the change, the issue no longer appears and the trace appears in the console.

Does This PR Require a Core Repo Change?

  • 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

@hangonlyra hangonlyra requested a review from a team February 9, 2023 18:49
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 9, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@hangonlyra hangonlyra changed the title Fix issue with Flask instrumentation when a request spawn children threads and copie the request context Fix issue with Flask instrumentation when a request spawn children threads and copies the request context Feb 9, 2023
@shalevr
Copy link
Member

shalevr commented Feb 9, 2023

Please add a changelog entry

@hangonlyra hangonlyra force-pushed the fix-flask-multithread-context-bug branch from 1676b09 to 76ebf6c Compare February 9, 2023 21:08
@hangonlyra
Copy link
Contributor Author

Please add a changelog entry

Updated. Thanks for the help.

@hangonlyra hangonlyra force-pushed the fix-flask-multithread-context-bug branch from 76ebf6c to 1e36cca Compare February 9, 2023 21:34
CHANGELOG.md Outdated Show resolved Hide resolved
@hangonlyra hangonlyra force-pushed the fix-flask-multithread-context-bug branch 2 times, most recently from 5009231 to b0db5ab Compare February 9, 2023 22:13
@shalevr
Copy link
Member

shalevr commented Feb 9, 2023

I am not familiar with flask requests,
Is there a legitimate way that the request starts with some thread and ends up in another thread?

@hangonlyra hangonlyra force-pushed the fix-flask-multithread-context-bug branch from b0db5ab to cd89ca9 Compare February 9, 2023 22:54
@hangonlyra
Copy link
Contributor Author

I am not familiar with flask requests, Is there a legitimate way that the request starts with some thread and ends up in another thread?

I can't think of any legitimate reason for that because it won't serve any useful purpose. It's definitely possible but it won't make any sense.

@hangonlyra hangonlyra force-pushed the fix-flask-multithread-context-bug branch from cd89ca9 to eb7a406 Compare February 9, 2023 23:02
@hangonlyra hangonlyra force-pushed the fix-flask-multithread-context-bug branch from 799e84e to 81e38e4 Compare February 13, 2023 17:33
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@matthewgrossman matthewgrossman left a comment

Choose a reason for hiding this comment

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

my approval in this repo doesn't mean anything, but just to echo from the initial issue that this will solve my issue

fixes #1551

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 for the informative PR description! I'm not super familiar with this but the solution makes sense to me after reading through.

How difficult would it be to add a unit test for this?

CHANGELOG.md Outdated Show resolved Hide resolved
shalevr and others added 2 commits February 15, 2023 14:45
@hangonlyra hangonlyra force-pushed the fix-flask-multithread-context-bug branch from 0237c2f to f063515 Compare February 15, 2023 17:48
@hangonlyra
Copy link
Contributor Author

@aabmass Good idea. Added new test case TestMultiThreading.test_multithreaded

@hangonlyra hangonlyra force-pushed the fix-flask-multithread-context-bug branch from f063515 to 4882ca4 Compare February 15, 2023 17:51
@srikanthccv srikanthccv enabled auto-merge (squash) February 15, 2023 18:25
auto-merge was automatically disabled February 15, 2023 18:43

Head branch was pushed to by a user without write access

@hangonlyra hangonlyra force-pushed the fix-flask-multithread-context-bug branch from 4882ca4 to 747924c Compare February 15, 2023 18:43
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.

Looks great! Thanks again

@hangonlyra hangonlyra force-pushed the fix-flask-multithread-context-bug branch from 747924c to 638c042 Compare February 16, 2023 16:27
CHANGELOG.md Outdated Show resolved Hide resolved
@hangonlyra hangonlyra force-pushed the fix-flask-multithread-context-bug branch from 075cff2 to efe9b8d Compare February 22, 2023 17:23
Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

Thanks

@srikanthccv srikanthccv enabled auto-merge (squash) February 22, 2023 20:52
@srikanthccv srikanthccv merged commit 74a8b90 into open-telemetry:main Feb 22, 2023
shalevr added a commit to shalevr/opentelemetry-python-contrib that referenced this pull request Feb 23, 2023
…/github.com/shalevr/opentelemetry-python-contrib into Change-metrics-tests-to-work-with-test_base

* 'Change-metrics-tests-to-work-with-test_base' of https://github.com/shalevr/opentelemetry-python-contrib:
  Fix issue with Flask instrumentation when a request spawn children threads and copies the request context (open-telemetry#1654)
  Add connection attributes to sqlalchemy connect span (open-telemetry#1608)
  Add boto3sqs to docs (open-telemetry#1666)
  Audit and test opentelemetry-instrumentation-elasticsearch NoOpTracer… (open-telemetry#1616)
  Copy change log updates from release/v1.16.x-0.37bx (open-telemetry#1683)
  Update version to 1.17.0.dev/0.38b0.dev (open-telemetry#1677)
  Fix CI Failure (open-telemetry#1680)
  Add better debugging if hatch subprocess fails (open-telemetry#1672)
  Add confluent kafka docs (open-telemetry#1668)
  Support aio_pika 9 (open-telemetry#1670)
  Audit and test opentelemetry-instrumentation-wsgi NoOpTracerProvider (open-telemetry#1610)
  bot (open-telemetry#1667)
  Add commit method for ConfluentKafkaInstrumentor's ProxiedConsumer (open-telemetry#1656)
  Revert open-telemetry#1097 (open-telemetry#1660)
  Audit and test opentelemetry-instrumentation-django NoOpTracerProvider (open-telemetry#1611)
  Audit and test opentelemetry-instrumentation-aiohttp-client NoOpTrace… (open-telemetry#1612)
  Audit and test opentelemetry-instrumentation-flask NoOpTracerProvider (open-telemetry#1614)
  Audit and test opentelemetry-instrumentation-dbapi NoOpTracerProvider (open-telemetry#1607)
pridhi-arora pushed a commit to pridhi-arora/opentelemetry-python-contrib that referenced this pull request Feb 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants