Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

starlette: Remove maximum version constraint#3456

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
xrmx merged 19 commits intoopen-telemetry:mainfromMattiasDC:issue/3317
May 23, 2025

Conversation

@MattiasDC
Copy link
Contributor

@MattiasDCMattiasDC commentedApr 29, 2025•
edited by xrmx
Loading

Description

Remove maximum version constraint on starlette, by fixing unit tests. Fixed some warnings in unit tests.

Problem was that the environment was patched only during thesetUp execution, and not during the tests themselves. By putting the patch decorator on the class itself, the environment variables are patched during the test class, as is done in another place in the same file.

This PR also includes some small changes to reduce the amount of warnings encountered in the unit tests.

  • Get rid of deprecated calls to theroute androute_websocket decorators
  • AsStarletteInstrumentor._uninstrument can clear all instrumented apps, use adiscard i.s.o.remove in the deletor of_InstrumentedStarlette to prevent aKeyError being raised

Fixes#3317

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?

All existing unit tests pass with the newest version of starlette (0.46.2)

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

Seecontributing.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

… unit tests. Fixed some warnings in unit tests.
@linux-foundation-easycla
Copy link

linux-foundation-easyclabot commentedApr 29, 2025•
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@MattiasDCMattiasDC requested a review froma team as acode ownerApril 29, 2025 20:06
@MattiasDC
Copy link
ContributorAuthor

@Kludex you mentioned you were interested in doing a review for#3317. If this is a bad time, would it be possible for you appoint someone else?

@MattiasDC
Copy link
ContributorAuthor

@aabmass could you review as well and are you able to upgrade the requirements? I'm not comfortable enough to update the lockfiles.

…trap/test-requirements. Use setUp/tearDown iso decorator on test class
@Kludex
Copy link
Member

Kludex commentedMay 2, 2025•
edited
Loading

We should test the min version.

@MattiasDC
Copy link
ContributorAuthor

MattiasDC commentedMay 2, 2025•
edited
Loading

We should test the min version.

Hi@Kludex thanks for joining,

Do you mean something else than this test:
'instrumentation-starlette-oldest 3.8 Ubuntu', which uses

Or am I missing something?

Kludex reacted with thumbs up emoji

@MattiasDC
Copy link
ContributorAuthor

@Kludex those oldest tests were indeed failing. I've increased the minimal version to 0.37.2. This one is a year old and didn't cause any problems with me. Is this acceptable? Older versions were causing this issue for meKludex/starlette#2770

@MattiasDCMattiasDC changed the titleISSUE-3317: Remove maximum version constraint on starlette, by fixing unit testsISSUE-3317: Remove maximum version constraint, increase minimum version on starlette by fixing unit testsMay 2, 2025
@MattiasDC
Copy link
ContributorAuthor

PR is ready to be reviewed again@emdneto, if anything needs a change, feel free to put me to work again ;)

@xrmxxrmx moved this toReviewed PR that needs fixing in@xrmx's Python PR digestMay 5, 2025
@xrmx
Copy link
Contributor

xrmx commentedMay 5, 2025

@Kludex those oldest tests were indeed failing. I've increased the minimal version to 0.37.2. This one is a year old and didn't cause any problems with me. Is this acceptable? Older versions were causing this issue for meencode/starlette#2770

That maybe be fixed by using an older httpx in the requirements.

@MattiasDC
Copy link
ContributorAuthor

MattiasDC commentedMay 6, 2025•
edited
Loading

@Kludex those oldest tests were indeed failing. I've increased the minimal version to 0.37.2. This one is a year old and didn't cause any problems with me. Is this acceptable? Older versions were causing this issue for meencode/starlette#2770

That maybe be fixed by using an older httpx in the requirements.

Conditional dependencies are not supported inrequirements.in files. Do you propose to add a.in file forlatest and one foroldest?

Signed-off-by: emdneto <9735060+emdneto@users.noreply.github.com>
Signed-off-by: emdneto <9735060+emdneto@users.noreply.github.com>
Signed-off-by: emdneto <9735060+emdneto@users.noreply.github.com>
Signed-off-by: emdneto <9735060+emdneto@users.noreply.github.com>
@emdnetoemdneto changed the titleISSUE-3317: Remove maximum version constraint, increase minimum version on starlette by fixing unit testsstarlette: Remove maximum version constraintMay 22, 2025
emdnetoand others added2 commitsMay 22, 2025 12:26
Signed-off-by: emdneto <9735060+emdneto@users.noreply.github.com>
Co-authored-by: Riccardo Magliocchetti <riccardo.magliocchetti@gmail.com>
Copy link
Contributor

@xrmxxrmx left a comment

Choose a reason for hiding this comment

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

With the changelog fixed

@xrmxxrmx requested a review fromemdnetoMay 23, 2025 12:20
@xrmxxrmxenabled auto-merge (squash)May 23, 2025 12:21
@xrmxxrmx merged commite2ba6d4 intoopen-telemetry:mainMay 23, 2025
720 checks passed
@github-project-automationgithub-project-automationbot moved this fromReviewed PR that needs fixing toDone in@xrmx's Python PR digestMay 23, 2025
@Kludex
Copy link
Member

I know this is not the right place, so please don't ask me to create an issue... But can't we somehow modify the starlette instrumentation to work with FastAPI and just have one instead of 2? It's a subclass anyway, and it seems it covers the same points.

Happy to talk or hack this at PyCon IT (cc@xrmx)

@xrmx
Copy link
Contributor

I know this is not the right place, so please don't ask me to create an issue... But can't we somehow modify the starlette instrumentation to work with FastAPI and just have one instead of 2? It's a subclass anyway, and it seems it covers the same points.

Happy to talk or hack this at PyCon IT (cc@xrmx)

Sure, we can discuss it at PyCon IT.

mxiamxia pushed a commit to aws-observability/aws-otel-python-instrumentation that referenced this pull requestOct 30, 2025
…58b0 (#524)This PR updates the upstream OpenTelemetry Python dependency to itsSeptember 2025 release; Upgrading from version 1.33.1/0.54b1 to1.37.0/0.58b0.It also resolves several conflicts between the following OTel PRs andexisting ADOT patches:starlette: Remove maximum version constraintopen-telemetry/opentelemetry-python-contrib#3456Make a BatchProcessor class which both BatchSpanRecordProcessor andBatchLogRecordProcessor can useopen-telemetry/opentelemetry-python#4562Make exporter timeout encompass retries/backoffs, add jitter tobackoffs, cleanup code a bitopen-telemetry/opentelemetry-python#4564Update BatchSpanProcessor to use new BatchProcessor classopen-telemetry/opentelemetry-python#4580Fix issue where deadlock can occur over logging._lockopen-telemetry/opentelemetry-python#4636Tests Performedtox -e linttox -e spellchecktox -e 3.9-test-aws-opentelemetry-distrotox -e 3.10-test-aws-opentelemetry-distrotox -e 3.11-test-aws-opentelemetry-distrotox -e 3.12-test-aws-opentelemetry-distrotox -e 3.13-test-aws-opentelemetry-distroSmoke/contract tests: ./gradlewappsignals-tests:contract-tests:contractTestsBy submitting this pull request, I confirm that you can use, modify,copy, and redistribute this contribution, under the terms of yourchoice.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@xrmxxrmxxrmx approved these changes

@emdnetoemdnetoemdneto approved these changes

Assignees

No one assigned

Labels

None yet

Projects

Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Starlette instrumentation tests fail recent versionsstarlette >= 0.15.0

4 participants

@MattiasDC@Kludex@xrmx@emdneto
[8]ページ先頭
©2009-2025 Movatter.jp