Skip to content
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

Vertex AI instrumentation boilerplate #3123

Merged
merged 4 commits into from
Dec 21, 2024

Conversation

aabmass
Copy link
Member

@aabmass aabmass commented Dec 20, 2024

Description

After the discussion on #3069, I decided to break this up into a series of PRs. This PR contains

  • Copy openai-v2 boilerplate and s/openai/vertexai
  • Updated the examples, READMEs, and docstrings to use VertexAI SDK
  • Update test-requirements-{0,1}.txt to pip Vertex AI dependencies for testing

Part of #3041

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Just boilerplate but I test the examples don't fail and new tests are passing with a placeholder

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

@aabmass aabmass force-pushed the vertex0-boilerplate branch from 6ee8502 to 17cbaac Compare December 20, 2024 06:20
@aabmass aabmass mentioned this pull request Dec 20, 2024
10 tasks
@aabmass aabmass marked this pull request as ready for review December 20, 2024 06:36
@aabmass aabmass requested a review from a team as a code owner December 20, 2024 06:36
@gyliu513
Copy link
Member

Nice boilerplate, thanks @aabmass ! I think all other instrumentations can refer to this PR as a template, and build their own instrumentation.

I think we probably need a README under https://github.com/open-telemetry/opentelemetry-python-contrib/tree/main/instrumentation-genai and add some tips for contribution etc, but we can probably create another PR to address this.

@karthikscale3
Copy link
Contributor

Nice work! thanks for adding this

@aabmass
Copy link
Member Author

aabmass commented Dec 20, 2024

Thanks for review!

I think we probably need a README under https://github.com/open-telemetry/opentelemetry-python-contrib/tree/main/instrumentation-genai and add some tips for contribution etc, but we can probably create another PR to address this.

We could add to this section https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/CONTRIBUTING.md#guideline-for-genai-instrumentations or break that all out and link is fine with me.

@aabmass aabmass enabled auto-merge (squash) December 21, 2024 23:41
@aabmass aabmass merged commit 72576f6 into open-telemetry:main Dec 21, 2024
584 checks passed
@aabmass aabmass deleted the vertex0-boilerplate branch December 21, 2024 23:50
@@ -0,0 +1,74 @@
# Copyright The OpenTelemetry Authors
Copy link
Member

Choose a reason for hiding this comment

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

@aabmass Happy New Year! May I know why did this instrumentation did not add -v2? We already have one in openllmetry https://github.com/traceloop/openllmetry/tree/gk/crewai-instrumentation/packages/opentelemetry-instrumentation-vertexai, ,thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

The current precedent of -v2 was to avoid package names in use by openllmetry

I asked the same on slack, from the angle of how we expect to publish this considering that. I was told that there is a negotiation about re-using the name currently in use by openllmetry (presumably to obviate theirs). I'm not sure if this is only about vertex, or all except openai or something other plan.

Those who can attend the weekly calls may be able to find out more on this topic, and please do share in an issue or issue comment the result of that!

Copy link
Member

Choose a reason for hiding this comment

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

Good, thanks @codefromthecrypt , I think we need a conclusion for this, we already have openai-v2 here, if we agree to use same name as openllmetry, we probably need remove -v2 for openai as well.

xrmx pushed a commit to xrmx/opentelemetry-python-contrib that referenced this pull request Jan 24, 2025
* Vertex AI boilerplate copied from openai-v2

* tox.ini boilerplate

* tox -e generate,generate-workflows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants