Skip to content

Starlette instrumentation's instruments version is too restrictive #3300

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
aabmass opened this issue Feb 25, 2025 · 4 comments · Fixed by #3304
Closed

Starlette instrumentation's instruments version is too restrictive #3300

aabmass opened this issue Feb 25, 2025 · 4 comments · Fixed by #3304
Assignees

Comments

@aabmass
Copy link
Member

aabmass commented Feb 25, 2025

The starlette instrumentation specifies

[project.optional-dependencies]
instruments = [
"starlette ~= 0.13.0",
]

This does not mean >=0.13, it actually means starlette>=0.13.0,<0.14.dev0. Concretely this overly narrow requirement is causing resolution issues with the new UV workspace introduced in #3124 and the google-genai instrumentation PR #3256.

...
      And because opentelemetry-instrumentation-starlette[instruments] depends on starlette>=0.13.0,<0.14.dev0 and opentelemetry-instrumentation-fastapi[instruments] depends on fastapi>=0.58, we can conclude that all of:
...
@aabmass
Copy link
Member Author

aabmass commented Feb 25, 2025

@Kludex @emdneto this is making tox -e ruff precommit fail, because of d092c1a

aabmass added a commit to michaelsafyan/open-telemetry.opentelemetry-python-contrib that referenced this issue Feb 26, 2025
@emdneto
Copy link
Member

emdneto commented Feb 26, 2025

@Kludex @emdneto this is making tox -e ruff precommit fail, because of d092c1a

Now the tox -e ruff doesn't make sense; we can probably change the name to pre-commit instead

@aabmass
Copy link
Member Author

aabmass commented Feb 26, 2025

Now the tox -e ruff doesn't make sense; we can probably change the name to pre-commit instead

I agree, but I think it might be a little annoying to people with muscle memory for tox -e ruff. Is it possible to keep tox -e ruff for just running ruff somehow?

@emdneto
Copy link
Member

emdneto commented Feb 27, 2025

@aabmass I think this, or create separated jobs for ruff, ruff-format and uv-lock.

diff --git a/tox.ini b/tox.ini
index c55482e7..f64bce06 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1006,6 +1006,18 @@ commands =
 
 [testenv:ruff]
 basepython: python3
+setenv =
+  SKIP=uv-lock
+deps =
+  -c {toxinidir}/dev-requirements.txt
+  pre-commit
+commands =
+  pre-commit run --color=always --all-files {posargs}

We can keep ruff in tox only and exclude it in workflows

@aabmass aabmass self-assigned this Feb 27, 2025
aabmass added a commit that referenced this issue Feb 27, 2025
…om/googleapis/python-genai) (#3256)

* Begin instrumentation of GenAI SDK.

* Snapshot current state.

* Created minimal tests and got first test to pass.

* Added test for span attributes.

* Ensure that token counts work.

* Add more tests.

* Make it easy to turn off instrumentation for streaming and async to allow for rapid iteration.

* Add licenses and fill out main README.rst.

* Add a changelog file.

* Fill out 'requirements.txt' and 'README.rst' for the manual instrumentation example.

* Add missing exporter dependency for the manual instrumentation example.

* Fill out rest of the zero-code example.

* Add minimal tests for async, streaming cases.

* Update sync test to use indirection on top of 'client.models.generate_content' to simplify test reuse.

* Fix ruff check issues.

* Add subproject to top-level project build mechanism.

* Simplify invocation of pylint.

* Fix 'make test' command and lint issues.

* Add '.dev' suffix to version per feedback on pull request #3256

* Fix README.rst files for the examples.

* Add specific versions for the examples.

* Revamp 'make test' to not require local 'tox.ini' configuration.

* Extend separators per review comment.

Co-authored-by: Riccardo Magliocchetti <[email protected]>

* Fix version conflict caused by non-hermetic requirements.

* Fix typo on the comment line.

* Add test for the use of the 'vertex_ai' system, and improve how this system is determined.

* Factor out testing logic to enable sharing with the async code.

* Addressed minor lint issues.

* Make it clearer that nonstreaming_base is a helper module that is not invoked directly.

* Integrate feedback from related pull request #3268.

* Update workflows with 'tox -e generate-workflows'.

* Improve data model and add some rudimentary type checking.

* Accept only 'true' for a true value to align with other code.

* Update the scope name used.

* Add **kwargs to patched methods to prevent future breakage due to the addition of future keyword arguments.

* Remove redundant list conversion in call to "sorted".

Co-authored-by: Aaron Abbott <[email protected]>

* Reformat with 'tox -e ruff'.

* Fix failing lint workflow.

* Fix failing lint workflow.

* Exclude Google GenAI instrumentation from the bootstrap code for now.

* Minor improvements to the tooling shell files.

* Fix typo flagged by codespell spellchecker.

* Increase alignment with broader repo practices.

* Add more TODOs and documentation to clarify the intended work scope.

* Remove unneeded accessor from OTelWrapper.

* Add more comments to the tests.

* Reformat with ruff.

* Change 'desireable' to 'desirable' per codespell spellchecker.

* Make tests pass without pythonpath

* Fix new lint errors showing up after change

* Revert "Fix new lint errors showing up after change"

This reverts commit 567adc6.

pylint ignore instead

* Add TODO item required/requested from code review.

Co-authored-by: Aaron Abbott <[email protected]>

* Simplify changelog per PR feedback.

* Remove square brackets from model name in span name per PR feedback.

* Misc test cleanup. Now that scripts are invoked solely through pytest via tox, remove main functions and hash bang lines.

* Improve quality of event logging.

* Update operation name to use a constant for consistency.

* Reformat with ruff.

* Exclude opentelemetry-instrumentation-google-genai from root uv workspace

Until #3300 is fixed.

---------

Co-authored-by: Riccardo Magliocchetti <[email protected]>
Co-authored-by: Aaron Abbott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants