Skip to content

fix excluded_urls in instrumentation-asgi#3567

Merged
xrmx merged 4 commits intoopen-telemetry:mainfrom
CarlosRCDev:fix-excluded_urls
Jul 1, 2025
Merged

fix excluded_urls in instrumentation-asgi#3567
xrmx merged 4 commits intoopen-telemetry:mainfrom
CarlosRCDev:fix-excluded_urls

Conversation

@CarlosRCDev
Copy link
Copy Markdown
Contributor

@CarlosRCDev CarlosRCDev commented Jun 9, 2025 •
edited
Loading

Description

If set value "excluded_urls" in OpenTelemetryMiddleware, in instrumentation-asgi. class fails: 'str' object has no attribute 'url_disabled'.

Fixes #3562
excluded_urls with parse_excluded_urls

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

In instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py

  • test_excluded_urls, fails if return span, with excluded_urls
  • test_no_excluded_urls, fails if not return span, with excluded_urls

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • [X ] Followed the style guidelines of this project
  • Unit tests have been added

@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Jun 9, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@CarlosRCDev CarlosRCDev mentioned this pull request Jun 9, 2025
Closed
emdneto
emdneto reviewed Jun 9, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

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

CI is failing. Maybe we should instead check if excluded_urls is already an ExcludeList?

@CarlosRCDev
Copy link
Copy Markdown
Contributor Author

CarlosRCDev commented Jun 9, 2025

CI is failing. Maybe we should instead check if excluded_urls is already an ExcludeList?

My fault, you are right, the problem:
the class OpenTelemetryMiddleware, when is used directly for example, quart or django, need a string to be convert in ExcludeList class (using parse_excluded_urls function), but the instrumentor like fastapi, instance the class OpenTelemetryMiddleware with excluded_urls as ExcludeList class.

The fix check if excluded_urls isistance of string, if not use the object without changes:

if isinstance(excluded_urls, str):
    excluded_urls = parse_excluded_urls(excluded_urls)

@emdneto
Copy link
Copy Markdown
Member

emdneto commented Jun 9, 2025

CI is failing. Maybe we should instead check if excluded_urls is already an ExcludeList?

My fault, you are right, the problem: the class OpenTelemetryMiddleware, when is used directly for example, quart or django, need a string to be convert in ExcludeList class (using parse_excluded_urls function), but the instrumentor like fastapi, instance the class OpenTelemetryMiddleware with excluded_urls as ExcludeList class.

The fix check if excluded_urls isistance of string, if not use the object without changes:

if isinstance(excluded_urls, str):
    excluded_urls = parse_excluded_urls(excluded_urls)

Right. Can you add a changelog entry?

@CarlosRCDev CarlosRCDev requested a review from a team as a code owner June 10, 2025 06:53
@CarlosRCDev
Copy link
Copy Markdown
Contributor Author

CarlosRCDev commented Jun 10, 2025

CI is failing. Maybe we should instead check if excluded_urls is already an ExcludeList?

My fault, you are right, the problem: the class OpenTelemetryMiddleware, when is used directly for example, quart or django, need a string to be convert in ExcludeList class (using parse_excluded_urls function), but the instrumentor like fastapi, instance the class OpenTelemetryMiddleware with excluded_urls as ExcludeList class.
The fix check if excluded_urls isistance of string, if not use the object without changes:

if isinstance(excluded_urls, str):
    excluded_urls = parse_excluded_urls(excluded_urls)

Right. Can you add a changelog entry?

Sorry for the wait, I just did it.

schneider8357
schneider8357 approved these changes Jun 10, 2025
View reviewed changes
Copy link
Copy Markdown

@schneider8357 schneider8357 left a comment

Choose a reason for hiding this comment

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

LGTM, the change is small and non-invasive. From what I understand, ExcludeList accepts Iterable[str], and it’s a common pattern to use strings in regex format for that. The helper function parse_excluded_urls splits the string accordingly, so the init function would now accept a string as well.
Two unit tests were added, covering the cases of an excluded URL and a non-excluded URL. I suggest adding tests with multiple exclusions in a single string, but I'm unsure if that is really necessary.

xrmx
xrmx reviewed Jun 30, 2025
View reviewed changes
Comment thread instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py
@xrmx xrmx moved this to Ready for review in Python PR digest Jun 30, 2025
@xrmx xrmx enabled auto-merge (squash) July 1, 2025 12:29
@xrmx xrmx merged commit b1c2c79 into open-telemetry:main Jul 1, 2025
625 checks passed
@github-project-automation github-project-automation Bot moved this from Ready for review to Done in Python PR digest Jul 1, 2025
devmonkey22 pushed a commit to devmonkey22/opentelemetry-python-contrib that referenced this pull request Aug 5, 2025
@CarlosRCDev @xrmx @devmonkey22
fix excluded_urls in instrumentation-asgi (open-telemetry#3567)
341ecd1 
* fix excluded_urls in instrumentation-asgi

* fix ExcludeList or str

* chhangelog

---------

Co-authored-by: Riccardo Magliocchetti <[email protected]>
sightseeker added a commit to sightseeker/opentelemetry-python-contrib that referenced this pull request Mar 11, 2026
@sightseeker @xrmx
fix excluded_urls in instrumentation-asgi (open-telemetry#3567)
26b3b07 
* fix excluded_urls in instrumentation-asgi

* fix ExcludeList or str

* chhangelog

---------

Co-authored-by: Riccardo Magliocchetti <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xrmx xrmx xrmx approved these changes

@emdneto emdneto emdneto approved these changes

+1 more reviewer

@schneider8357 schneider8357 schneider8357 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

Python PR digest
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

(excluded_urls) 'str' object has no attribute 'url_disabled' in OpenTelemetryMiddleware

4 participants

@CarlosRCDev @emdneto @xrmx @schneider8357