Skip to content

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

Merged
codeboten merged 5 commits intoopen-telemetry:masterfrom
closeio:fix-flask-test-request-context
Dec 15, 2020
Merged

When Flask activation is missing, do not emit a log message#253
codeboten merged 5 commits intoopen-telemetry:masterfrom
closeio:fix-flask-test-request-context

Conversation

@jpmelos
Copy link
Contributor

@jpmelos jpmelos commented Dec 12, 2020

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

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 review from a team, hectorhdzg and toumorokoshi and removed request for a team December 12, 2020 18:44
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 12, 2020

CLA Signed

The committers are authorized under a signed CLA.

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
([#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

Labels

None yet

Projects

None yet

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