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 example in documentation #551

Merged
merged 4 commits into from
Apr 8, 2020
Merged

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Apr 4, 2020

Fixes #550

This fix requires to run code before an import statement, which is frowned upon by PEP8 format checkers because this is considered an error (E402). Nevertheless, I prefer to add this as a fix now in order to have at least a functional example. Avoiding this error will require more functionality added to the instrumentor.

@ocelotl ocelotl requested a review from a team April 4, 2020 02:45
@ocelotl ocelotl self-assigned this Apr 4, 2020
@ocelotl ocelotl added doc Documentation-related ext labels Apr 4, 2020
@AntonKueltz
Copy link

AntonKueltz commented Apr 4, 2020

Out of curiosity, was there a reason the flask instrumentation interface was changed? The previous version of having a function that takes the flask app as a param which can be called in the flask apps bootstrap method feels closer to the usual pattern of how other flask extensions are integrated. Imo having an instrumentation class that has to be called in a specific order and relies on side effects feels a lot less clear for someone consuming / integrating the extension.

@ocelotl
Copy link
Contributor Author

ocelotl commented Apr 6, 2020

Out of curiosity, was there a reason the flask instrumentation interface was changed? The previous version of having a function that takes the flask app as a param which can be called in the flask apps bootstrap method feels closer to the usual pattern of how other flask extensions are integrated. Imo having an instrumentation class that has to be called in a specific order and relies on side effects feels a lot less clear for someone consuming / integrating the extension.

Hello

This was changed because we introduced support for auto instrumentation in #327. With this new feature came a standard API for instrumentors (the instrument method called in the example is part of this API).

This being said, having to call instrument before we import flask is definitely not the approach we want to take from now on. This is of course very fragile, breaks PEP8 and very hard to debug if it causes an error. I'll be opening an issue to track the effort to find a solution for this problem.

@lzchen
Copy link
Contributor

lzchen commented Apr 7, 2020

I believe docs\getting-started.rst also has an example that needs to be fixed.

Copy link
Member

@c24t c24t left a comment

Choose a reason for hiding this comment

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

@ocelotl if e4a741c LGTY I'll merge this.

I agree we need a more idiomatic way to do this, and one that doesn't rely on import order effects.

@ocelotl
Copy link
Contributor Author

ocelotl commented Apr 8, 2020

@ocelotl if e4a741c LGTY I'll merge this.

I agree we need a more idiomatic way to do this, and one that doesn't rely on import order effects.

LGTM, fire away 👍

@c24t c24t merged commit 91cdfe7 into open-telemetry:master Apr 8, 2020
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Documentation-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flask instrumentation example doesn't work
4 participants