Enable passing explicit urls to exclude in instrumentation in FastAPI #486

cdvv7788 · 2021-05-02T18:04:41Z

Description

Proposal to solve #375
Currently, only environment variables are supported to exclude URLs. With this change, the users can pass a list explicitly during the instrumentation, so other sources can be easily plugged in (hardcoded, config files, settings, etc).

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?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Test A: Instrument the app manually passing the excluded_urls list. Verify that no traces are being generated for those urls.
Test B: Instrument app automatically (using instrument). You can pass the explicit_urls list to it so. Verify that no traces are being generated for those urls

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

… fastapi

linux-foundation-easycla · 2021-05-02T18:04:44Z

The committers are authorized under a signed CLA.

✅ Cristian Vargas (f728abb, 8c4eee0)

owais

Suggest to rename the global _excluded_urls to _excluded_urls_from_env. It'd make the logic below a lot more readable.

… instrumentation to make implementation more readable

owais · 2021-05-02T19:08:08Z

.../opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py

@@ -47,6 +53,7 @@ def instrument_app(app: fastapi.FastAPI, tracer_provider=None):
    def _instrument(self, **kwargs):
        self._original_fastapi = fastapi.FastAPI
        _InstrumentedFastAPI._tracer_provider = kwargs.get("tracer_provider")
+        _InstrumentedFastAPI._excluded_urls = kwargs.get("excluded_urls")


Since _INstrumentedFastAPI is private and not supposed to be used directly, can we move env vs kwargs logic here instead?

lzchen · 2021-05-04T16:15:27Z

instrumentation/opentelemetry-instrumentation-fastapi/README.rst

+
+.. code-block:: python
+    from opentelemetry.util.http import ExcludeList
+    FastAPIInstrumentor.instrument_app(app, ExcludeList("client/.*/info,healthcheck"))


Why not allow users to pass in a sequence of strings instead? Basically what can be passed into the env var should be replicated here.

No real reason, I can change it to work that way.

I think it is preferred. Also users don't have to import ExcludeList either.

lzchen · 2021-05-04T16:19:53Z

@cdvv7788
Are you planning on adding this feature to the rest of the url related instrumentations? If not, I do not believe the original issue is scoped properly. Please create another issue to add this for the remaining instrumentations, and note that FastAPI has already implemented this.

…api)

cdvv7788 · 2021-05-09T17:46:11Z

@lzchen I created a new issue to track this in a more general way: #490

I updated the PR with the recommendations. I still have an issue: https://github.com/open-telemetry/opentelemetry-python-contrib/pull/486/checks?check_run_id=2502217693
That seems to be related to sphinx. What am I doing wrong in there?

lzchen · 2021-05-10T16:27:59Z

@owais
Are your questions addressed?

lzchen · 2021-05-10T16:48:51Z

.../opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py

+_excluded_urls_from_env = get_excluded_urls("FASTAPI")
+
+
+def parse_urls(urls_str):


Private method

I will move it here: https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py#L55
This will be needed for other instrumentations

instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py

instrumentation/opentelemetry-instrumentation-fastapi/README.rst

Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>

cdvv7788 · 2021-05-30T16:52:04Z

@ocelotl @lzchen Resolved your comments. Please review.

util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py

codeboten

The PR looks good, if you could take a look at fixing the tests, would be happy to merge this as soon as that's done. Thanks for the contribution!

codeboten · 2021-06-07T15:37:10Z

.../opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py

+        _InstrumentedFastAPI._excluded_urls = (
+            _excluded_urls_from_env
+            if _excluded_urls is None
+            else parse_urls(_excluded_urls)


Should this be parse_excluded_urls?

Yes, it should. My bad.
The linter should be passing now.

…open-telemetry#486)

feat: Enable passing explicit urls to exclude from instrumentation in…

f728abb

… fastapi

cdvv7788 requested review from a team, owais and srikanthccv and removed request for a team May 2, 2021 18:04

cdvv7788 changed the title ~~Enable passing explicit urls to exclude from instrumentation in FastAPI~~ Enable passing explicit urls to exclude in instrumentation in FastAPI May 2, 2021

docs: Update changelog (fastapi open-telemetry#486)

8c4eee0

owais reviewed May 2, 2021

View reviewed changes

refactor: Rename _excluded_urls to _excluded_urls_from_env in fastapi…

17a0269

… instrumentation to make implementation more readable

owais reviewed May 2, 2021

View reviewed changes

refactor: Move excluded urls logic to the _instrument function

f047153

lzchen reviewed May 4, 2021

View reviewed changes

Merge branch 'main' into fastapi-excluded_urls

58c20cc

refactor: Use ExcludeList only internally in the instrumentation(fast…

b81dbf5

…api)

cdvv7788 mentioned this pull request May 9, 2021

Enable all the http server instrumentations to directly receive the excluded urls as a param #490

Open

Merge branch 'main' into fastapi-excluded_urls

eea46e4

lzchen reviewed May 10, 2021

View reviewed changes

Merge branch 'main' into fastapi-excluded_urls

4ce59f2

owais approved these changes May 11, 2021

View reviewed changes

lzchen and others added 2 commits May 12, 2021 08:59

Merge branch 'main' into fastapi-excluded_urls

178643d

Merge branch 'main' into fastapi-excluded_urls

314d329

ocelotl suggested changes May 26, 2021

View reviewed changes

instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py Outdated Show resolved Hide resolved

ocelotl suggested changes May 26, 2021

View reviewed changes

instrumentation/opentelemetry-instrumentation-fastapi/README.rst Outdated Show resolved Hide resolved

cdvv7788 and others added 3 commits May 30, 2021 11:41

refactor: Move utils method into http-utils package

a335531

test: Remove unneeded variable in explicit excluded_urls test

34fa727

fix: Resolve docs generation issue

b659f08

Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>

cdvv7788 requested review from ocelotl and lzchen May 30, 2021 16:52

owais reviewed May 30, 2021

View reviewed changes

util/opentelemetry-util-http/src/opentelemetry/util/http/__init__.py Outdated Show resolved Hide resolved

fix: Remove unneeded import

180031e

ocelotl approved these changes Jun 1, 2021

View reviewed changes

lzchen approved these changes Jun 2, 2021

View reviewed changes

cdvv7788 and others added 2 commits June 2, 2021 16:31

Merge branch 'main' into fastapi-excluded_urls

7bce354

Merge branch 'main' into fastapi-excluded_urls

59a504c

codeboten suggested changes Jun 7, 2021

View reviewed changes

cdvv7788 and others added 2 commits June 13, 2021 10:39

fix: wrong variable name in fastapi instrumentation

02fa810

Merge branch 'main' into fastapi-excluded_urls

b495dfe

codeboten approved these changes Jun 14, 2021

View reviewed changes

Merge branch 'main' into fastapi-excluded_urls

fbaa815

lzchen merged commit 7cae4d3 into open-telemetry:main Jun 15, 2021

cdvv7788 deleted the fastapi-excluded_urls branch June 15, 2021 03:33

andresbeckruiz pushed a commit to open-o11y/opentelemetry-python-contrib that referenced this pull request Jun 25, 2021

Enable passing explicit urls to exclude in instrumentation in FastAPI (…

233dc91

…open-telemetry#486)

mattoberle mentioned this pull request Jul 22, 2021

Provide excluded_urls argument to Flask instrumentation #604

Merged

11 tasks

ivergara mentioned this pull request Mar 29, 2024

Provide instrumentations a way to exclude_urls that is not using environment variables #375

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable passing explicit urls to exclude in instrumentation in FastAPI #486

Enable passing explicit urls to exclude in instrumentation in FastAPI #486

cdvv7788 commented May 2, 2021 •

edited

Loading

linux-foundation-easycla bot commented May 2, 2021 •

edited

Loading

owais left a comment

owais May 2, 2021

lzchen May 4, 2021

cdvv7788 May 4, 2021

lzchen May 5, 2021

cdvv7788 May 9, 2021

lzchen commented May 4, 2021

cdvv7788 commented May 9, 2021

lzchen commented May 10, 2021

lzchen May 10, 2021

cdvv7788 May 11, 2021

cdvv7788 commented May 30, 2021

codeboten left a comment

codeboten Jun 7, 2021

cdvv7788 Jun 13, 2021

		_excluded_urls_from_env = get_excluded_urls("FASTAPI")


		def parse_urls(urls_str):

Enable passing explicit urls to exclude in instrumentation in FastAPI #486

Enable passing explicit urls to exclude in instrumentation in FastAPI #486

Conversation

cdvv7788 commented May 2, 2021 • edited Loading

Description

Type of change

How Has This Been Tested?

Does This PR Require a Core Repo Change?

Checklist:

linux-foundation-easycla bot commented May 2, 2021 • edited Loading

owais left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzchen commented May 4, 2021

cdvv7788 commented May 9, 2021

lzchen commented May 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdvv7788 commented May 30, 2021

codeboten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdvv7788 commented May 2, 2021 •

edited

Loading

linux-foundation-easycla bot commented May 2, 2021 •

edited

Loading