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

Include Flask 2.0 as compatible with existing flask instrumentation #545

Conversation

NathanielRN
Copy link
Contributor

Description

opentelemetry-instrumentation-flask uses flask.Flask to instrument the flask package. Initially instrumentation worked for flask~=1.0. About a month ago, Flask 2.0 was released and thankfully our instrumentation still works with it!

We need to manually update package.py to indicate that our instrumentation still works for auto-instrumentation. Auto-instrumentation will only instrumentation packages and versions which match what we have in instrumentations/.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

The packages found by Auto Instrumentation can be found by running opentelemetry-bootstrap after installing pip opentelemetry-instrumentation.

Before the change:

If you have pip install flask~=1.0 then running opentelemetry-bootstrap returns at least:

opentelemetry-instrumentation-flask==0.22b0

If you have pip install flask ~=2.0 then running opentelemetry-bootstrap returns nothing.

After the change:

Both pip install flask~=1.0 and pip install flask~=2.0 will make running opentelemetry-bootstrap return at least:

opentelemetry-instrumentation-flask==0.22b0

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

@NathanielRN NathanielRN requested review from a team, codeboten and srikanthccv and removed request for a team June 21, 2021 20:20
@NathanielRN NathanielRN changed the title Include Flask 2.0 as compatible with flask instrumentation Include Flask 2.0 as compatible with existing flask instrumentation Jun 21, 2021
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Any thoughts on how to approach testing the various versions @NathanielRN? It would be great to have a test for each supported versions, at least for major versions.

@srikanthccv
Copy link
Member

I see some newly added async support in the changelog for version 2.0. I think we should have some tests around such major changes.

@NathanielRN
Copy link
Contributor Author

@codeboten That's a good question. I was thinking about it especially since we got lucky this time that there were no breaking changes.

If, for example, Flask 3 breaks opentelemetry-python-flask we could:

  1. Publish a opentelemetry-instrumentation-flask-v2 ?
  2. Have one opentelemetry-instrumentation-flask package that detects the installed pkg and instruments accordingly? (Hard to test?)

Then in terms of testing, we could:

  1. Separate the tests between the packages
  2. Put all the tests for all the versions in one package, and our tox.ini file could rename flask to flask1-2 and add a flask3- which installs Flask 3.

I favor Option 2, it also has the added side effect that in tox, all common commands between the flask#-# tests could be matched with just the flask keyword.

@lonewolf3739 I tried for several hours today to try to get async tests to work but with little success 😕 I tried looking at some of the other examples of async tests that we have but I can't find an easy way to integrate them with the flask tests that already we have. But, I would say it's okay to skip the tests for now because:

  1. Using the async/await feature requires downloading the "async" extra pip install flask[async]
  2. It's a very new feature with few examples online of people using it (i.e. it's been popular for a long time without this feature!)
  3. We use from werkzeug.test import Client to create a client that calls the app, but they don't have an AsyncClient again possibly because it's so new?

The flask library introduced tests using pytest but I wasn't able to get their examples to work for us.

As a last point, I tested this with a very simple app and found that it does work even with an async endpoint 😃

The async stuff can be used to make an endpoint async very easily like this:

import asyncio
...
@app.route("/foo")
async def call_foo_endpoint():
    await asyncio.sleep(0)
    requests.get("https://aws.amazon.com/")

    return '{{"traceId": "{}"}}'.format(trace.get_current_span().get_span_context().trace_id)

Even with async/await I can get traces reliably. Our tests test the basics and I assume that anything that would break the async/await routes would break the sync ones too so it can be said that our tests cover the main case and that's enough for what we care about as OTel.

If we're sure we want tests, then I can close this PR and open it again at a later time when I figure out how to test it 😅

@NathanielRN
Copy link
Contributor Author

@codeboten I had a chance to ask OTel Java how they're doing auto instrumentation and they lead me to this example:

Which means they go with Option 1 that I mentioned instead. This means tests would be separate and possible duplicated across packages. (Notice how they skipped apache-httpclient--3.0 presumably because the instrumentation package didn't need to be updated for v3.0)

I think it makes a lot of sense and could fit with OTel Python auto instrumentation nicely. It's just that we would have to deprecate opentelemetry-instrumentation-flask in favor of opentelemetry-instrumentation-flask-1.0 and do that for every instrumentation package in the Contrib repo 😓

@srikanthccv
Copy link
Member

This problem is common across instrumentations. I think this deserves a separate discussion on our approach towards addressing this. This will also need effort in terms of tooling, maintaining, and release, etc.

@NathanielRN
Copy link
Contributor Author

@lonewolf3739 Sure I would love to add what I've discovered to any discussion. In the meantime for this PR it looks like we already follow this pattern of allowing multiple versions be supported by one package?

We can bring this up at the SIG to decide whether we want this specific PR or not.

@codeboten codeboten merged commit 9c81eda into open-telemetry:main Jun 24, 2021
@NathanielRN NathanielRN deleted the bump-flask-instrumentation-compatability branch July 15, 2021 16:18
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.

3 participants