Skip to content

[chore] Replace '-0' and '-1' with semantically meaningful suffixes. Other minor fixes in 'instrumentation-genai' test structure. #3268

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Feb 19, 2025
Merged

[chore] Replace '-0' and '-1' with semantically meaningful suffixes. Other minor fixes in 'instrumentation-genai' test structure. #3268

merged 5 commits into from
Feb 19, 2025

Conversation

@michaelsafyan
Copy link
Contributor

@michaelsafyan michaelsafyan commented Feb 13, 2025

Description

This is a follow up item from feedback on:

In particular, the new code proposed here breaks with the existing pattern of adding -0 and -1 suffixes in the tox.ini file in favor of more intuitive -old-deps and -recent-deps suffixes, encoding the intended meaning.

It was requested on that PR to send a separate PR to make similar updates to the other cases.

This updates the existing instrumentation-genai packages to follow a similar pattern. It likewise updates the naming and location of the corresponding requirements.txt files to similarly reflect their meaning. And it adds documentation comments to the requirements.txt files that parallel those in that pull request.

Type of change

Minor internal structuring, build improvement.

How Has This Been Tested?

Ran tox to verify that things still build. In particular, ran the following commands:

  • tox -l | grep openai | xargs -n 1 tox -e
  • tox -l | grep vertexai | xargs -n 1 tox -e

I verified that both recent-deps and old-deps variants of each pass for at least one version of Python, verifying that this modification did not induce any breakages in the general build mechanism.

Does This PR Require a Core Repo Change?

No

Checklist:

I believe that all checklist items that apply have been followed.

@michaelsafyan michaelsafyan requested a review from a team as a code owner February 13, 2025 19:36
@michaelsafyan michaelsafyan mentioned this pull request Feb 13, 2025
Merged
2 tasks
@michaelsafyan
Copy link
Contributor Author

michaelsafyan commented Feb 13, 2025

Note: the feedback on that PR that I'm referring to is this:

#3256 (comment)

@michaelsafyan
Copy link
Contributor Author

michaelsafyan commented Feb 13, 2025

CC: @aabmass

aabmass
aabmass approved these changes Feb 13, 2025
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Thanks! I'd like one more review from other maintainers.

@codefromthecrypt
Copy link
Contributor

codefromthecrypt commented Feb 13, 2025

I think in a diff like this best to use two words and be consistent both in tox and also the requirements names.

So, "latest" makes sense, so in both places.

For the floor version I suggest using "floor" or similar and also both places.

Basically, I think that whatever the two words chosen this PR passes when there are 2 not 4 phrases for the two intents, verified that diffs match renames

My 2p

emdneto
emdneto reviewed Feb 14, 2025
View reviewed changes
@emdneto emdneto added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Feb 14, 2025
michaelsafyan added a commit to michaelsafyan/open-telemetry.opentelemetry-python-contrib that referenced this pull request Feb 18, 2025
@michaelsafyan
Integrate feedback from related pull request open-telemetry#3268.
97494b5
xrmx
xrmx approved these changes Feb 19, 2025
View reviewed changes
Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

Not a fan of adding copyright headers to requirements files but LGTM

@xrmx
Copy link
Contributor

xrmx commented Feb 19, 2025 •
edited
Loading

Updating repo settings to have the proper jobs required to pass

@xrmx xrmx merged commit 0cb9ba5 into open-telemetry:main Feb 19, 2025
694 checks passed
aabmass added a commit that referenced this pull request Feb 27, 2025
@michaelsafyan @xrmx @aabmass
Initial version of instrumentation for the Google GenAI SDK (github.c…
c09a299 
…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 <riccardo.magliocchetti@gmail.com>

* 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 <aaronabbott@google.com>

* 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 <aaronabbott@google.com>

* 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 <riccardo.magliocchetti@gmail.com>
Co-authored-by: Aaron Abbott <aaronabbott@google.com>
aabmass added a commit that referenced this pull request Mar 3, 2025
@michaelsafyan @xrmx @aabmass
Add support for async and streaming responses in the Google GenAI ins…
81eaea5 
…trumentation (#3298)

* 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 <riccardo.magliocchetti@gmail.com>

* 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 <aaronabbott@google.com>

* 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 <aaronabbott@google.com>

* Simplify changelog per PR feedback.

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

* Checkpoint current state.

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

* Implement streaming support in RequestsMocker, get tests passing again.

* Add test with multiple responses.

* Remove support for async and streaming from TODOs, since this is now addressed.

* Increase testing coverage for streaming.

* Reformat with ruff.

* Add minor version bump with changelog.

* Change TODOs to bulleted list.

* Update per PR feedback

Co-authored-by: Aaron Abbott <aaronabbott@google.com>

* Restructure streaming async logic to begin execution earlier.

* Reformat with ruff.

* Disable pylint check for catching broad exception. Should be allowed given exception is re-raised.

* Simplify async streaming solution per PR comment.

---------

Co-authored-by: Riccardo Magliocchetti <riccardo.magliocchetti@gmail.com>
Co-authored-by: Aaron Abbott <aaronabbott@google.com>
@michaelsafyan michaelsafyan mentioned this pull request Mar 18, 2025
Closed
6 tasks
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

@aabmass aabmass aabmass approved these changes

@emdneto emdneto emdneto approved these changes

@alizenhom alizenhom Awaiting requested review from alizenhom

@codefromthecrypt codefromthecrypt Awaiting requested review from codefromthecrypt

@gyliu513 gyliu513 Awaiting requested review from gyliu513

@karthikscale3 karthikscale3 Awaiting requested review from karthikscale3

@lmolkova lmolkova Awaiting requested review from lmolkova

@lzchen lzchen Awaiting requested review from lzchen

@nirga nirga Awaiting requested review from nirga

Assignees

@codefromthecrypt codefromthecrypt

@lmolkova lmolkova

@nirga nirga

@gyliu513 gyliu513

@lzchen lzchen

@karthikscale3 karthikscale3

@alizenhom alizenhom

Labels

Skip Changelog PRs that do not require a CHANGELOG.md entry

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.