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 Falcon instrumentation compatible with Falcon >=3 #607

Merged
merged 12 commits into from
Aug 24, 2021

Conversation

adriangb
Copy link
Contributor

@adriangb adriangb commented Jul 27, 2021

Replace falcon.api.Request -> falcon.Request which exists on both Falcon 2 & 3

Steps to reproduce:

  1. Install Falcon >=3: pip install falcon==3.0.0
  2. Run the docs: https://opentelemetry-python-contrib.readthedocs.io/en/latest/instrumentation/falcon/falcon.html#usage

You'll get:

.../dist-packages/opentelemetry/instrumentation/falcon/__init__.py in <module>()
--> 129 _response_propagation_setter = FuncSetter(falcon.api.Response.append_header)
AttributeError: module 'falcon' has no attribute 'api'

And I verified that in Falcon >=3 falcon.api does not exit.

Also to note, falcon.API is being deprecated in favor of falcon.App. Not sure how you want to handle that. I reckon some version checks will be needed to support both Falcon 2 & 4.

Replace falcon.api.Request -> falcon.Request which exists on both Falcon 2 & 3
@adriangb adriangb requested review from a team, lzchen and srikanthccv and removed request for a team July 27, 2021 18:34
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 31, 2021

CLA Signed

The committers are authorized under a signed CLA.

@owais
Copy link
Contributor

owais commented Jul 31, 2021

@adriangb Thanks for the PR. You'll also need to update the supported Falcon versions here:

Also please sign the CLA and update the changelog.

@adriangb
Copy link
Contributor Author

Thanks @owais . CLA link is giving me a 500, I opened a support ticket.

I edited the package and changelog. Will updating the package also run tests on 3.0?

@owais
Copy link
Contributor

owais commented Aug 2, 2021

I edited the package and changelog. Will updating the package also run tests on 3.0?

No. You'll need to update the test matrix in tox.ini to do that.

@adriangb
Copy link
Contributor Author

adriangb commented Aug 2, 2021

@owais CLA issue is fixed and signed, and I updated tox.ini, although I couldn't test locally because as per #444 the CONTRIBUTING.md instructions are broken. Can you please kick off CI?

@owais
Copy link
Contributor

owais commented Aug 2, 2021

It is triggered automatically on every push. Looks like the falcon tests are failing.

@adriangb
Copy link
Contributor Author

adriangb commented Aug 2, 2021

It is triggered automatically on every push.

Not if you're a first time contributor:

image

Looks like the falcon tests are failing.

I reverted the changes to tox.ini, there were version conflicts in the env. I guess I'll see if the current tests run or not (since I had not seen them run before because, as per above, workflows won't run for me)

I'd appreciate a bit more hands-on support here, the interdependency between the otel packages (api, sdk, core, contrib, etc.) and the packaging of the repo (monorepo, tox, etc.) compounded with broken contributing instructions make it a bit confusing for a first time contributor. It's taking a lot more work than I'd like to contribute a 1 line fix.

@owais owais self-assigned this Aug 19, 2021
@adriangb
Copy link
Contributor Author

@owais I saw you self assigned this. Will you be taking it over? I don't want us to duplicate work offline 😄

@owais
Copy link
Contributor

owais commented Aug 23, 2021

@adriangb No, a PR assignee represents someone who has taken up the responsibility to review a PR and see it through. I don't intent to work on it :)

@adriangb
Copy link
Contributor Author

adriangb commented Aug 23, 2021

Got it, thanks for clarifying. I'm going to pick this up again this week.

It does look like there's still some infrastructure / CI issues: I'm seeing failures in Celary tests and Linting timing out, which certainly aren't related to this change.

@owais
Copy link
Contributor

owais commented Aug 24, 2021

We should update tox.ini to test multiple versions but not a blocker for me rigth now.

@adriangb
Copy link
Contributor Author

Yep I was just waiting to see if the current failures would resolve before changing more stuff. Looks like the last run passed, so I'll try to put this change in again.

@owais owais self-requested a review August 24, 2021 22:27
@owais owais merged commit bfaabbf into open-telemetry:main Aug 24, 2021
@adriangb adriangb deleted the patch-1 branch August 24, 2021 22:40
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