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.

Sign up for GitHub

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

Jump to bottom

When Flask activation is missing, do not emit a log message #253

Merged
merged 5 commits into from
Dec 15, 2020
Merged

When Flask activation is missing, do not emit a log message #253

merged 5 commits into from
Dec 15, 2020

Conversation

jpmelos
Copy link
Contributor

@jpmelos jpmelos commented Dec 12, 2020 •
edited
Loading

Description

If a Flask request doesn't have an active span, it just means that it was initialized via a mechanism that doesn't run before_request, like app.test_request_context or even manually. It is okay and instrumentation still works.

Fixes #160.

Type of change

  • 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?

I ran the same script that is in #160 and I didn't get the warning message.

Does This PR Require a Core Repo Change?

  • Yes
  • 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 Not needed
  • Documentation has been updated Not needed

@jpmelos
When Flask activation is missing, do not emit a log message
66bb413 
If a Flask request doesn't have an active span, it just means that it
was initialized via a mechanism that doesn't run `before_request`, like
`app.test_request_context` or even manually. It is okay and
instrumentation still works.
@jpmelos jpmelos requested a review from a team as a code owner December 12, 2020 18:44
@jpmelos jpmelos requested review from toumorokoshi and hectorhdzg and removed request for a team December 12, 2020 18:44
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 12, 2020 •
edited
Loading

CLA Signed

The committers are authorized under a signed CLA.

@jpmelos jpmelos mentioned this pull request Dec 12, 2020
Merged
codeboten
codeboten approved these changes Dec 14, 2020
View reviewed changes
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.

Thanks for the PR! The branch needs updating and I added a comment regarding where in the changelog this should be. Once the branch is updated we can merge!

CHANGELOG.md Outdated
@@ -21,6 +21,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#212](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/212))
- `opentelemetry-instrumentation-fastapi` Added support for excluding some routes with env var `OTEL_PYTHON_FASTAPI_EXCLUDED_URLS`
([#237](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/237))
- `opentelemetry-instrumentation-flask` Do not emit a warning message for request contexts created with `app.test_request_context` or manually.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably go under Changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 7a97ccd.

@jpmelos
Copy link
Contributor Author

jpmelos commented Dec 15, 2020

@codeboten since my changes are a merge of master into this branch and an edit to a .md file, I have no idea how two checks failed when they had previously passed. Is there any chance those failures are intermittent and retrying would suffice? I can't find the retry button, so maybe I lack permission to retry actions?

@lzchen lzchen closed this Dec 15, 2020
@lzchen lzchen reopened this Dec 15, 2020
@lzchen
Copy link
Contributor

lzchen commented Dec 15, 2020

@jpmelos
Can you try rebasing from master? The build should be fixed.

@jpmelos
Copy link
Contributor Author

jpmelos commented Dec 15, 2020

@lzchen all checks passed now.

@codeboten codeboten merged commit 20c1cb9 into open-telemetry:master Dec 15, 2020
@jpmelos jpmelos deleted the fix-flask-test-request-context branch December 15, 2020 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeboten codeboten codeboten approved these changes

@lzchen lzchen lzchen approved these changes

@toumorokoshi toumorokoshi Awaiting requested review from toumorokoshi

@hectorhdzg hectorhdzg Awaiting requested review from hectorhdzg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Flask instrumentor logs an error message when used with app.test_request_context
3 participants
@jpmelos @lzchen @codeboten