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.

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

Add Initial Support for Instrumenting OpenAI Python Library - Chat Completion Create #2759

Merged
merged 53 commits into from
Oct 22, 2024
Merged

Add Initial Support for Instrumenting OpenAI Python Library - Chat Completion Create #2759

merged 53 commits into from
Oct 22, 2024

Conversation

karthikscale3
Copy link
Contributor

@karthikscale3 karthikscale3 commented Jul 31, 2024 •
edited
Loading

Description

This PR adds support for tracing the official python openai library.

This pull request introduces initial support for instrumenting the OpenAI Python library, specifically targeting the chat.completion.create method. This implementation aligns with the GenAI semantic conventions.

Fixes #1796

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?

This is a work in progress PR at the moment and I plan to add unit tests and update this space.

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

@karthikscale3 karthikscale3 marked this pull request as draft July 31, 2024 07:15
xrmx
xrmx reviewed Jul 31, 2024
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.

Added a first round of comments, thanks!

instrumentation/opentelemetry-instrumentation-openai/README.rst Outdated Show resolved Hide resolved
instrumentation/opentelemetry-instrumentation-openai/pyproject.toml Outdated Show resolved Hide resolved
instrumentation/opentelemetry-instrumentation-openai/pyproject.toml Outdated Show resolved Hide resolved
...on/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/__init__.py Outdated Show resolved Hide resolved
...telemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/span_attributes.py Outdated Show resolved Hide resolved
...on/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/__init__.py Outdated Show resolved Hide resolved
instrumentation/opentelemetry-instrumentation-openai/pyproject.toml Outdated Show resolved Hide resolved
...ation/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/patch.py Outdated Show resolved Hide resolved
...ation/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/patch.py Outdated Show resolved Hide resolved
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Aug 7, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

lmolkova
lmolkova reviewed Aug 8, 2024
View reviewed changes
...ation/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/patch.py Outdated Show resolved Hide resolved
...ation/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/patch.py Outdated Show resolved Hide resolved
...ation/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/patch.py Outdated Show resolved Hide resolved
...ation/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/patch.py Outdated Show resolved Hide resolved
...ation/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/patch.py Outdated Show resolved Hide resolved
...ation/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/patch.py Outdated Show resolved Hide resolved
...ation/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/patch.py Outdated Show resolved Hide resolved
...ation/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/patch.py Outdated Show resolved Hide resolved
...ation/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/patch.py Outdated Show resolved Hide resolved
...ation/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/patch.py Outdated Show resolved Hide resolved
lmolkova
lmolkova reviewed Aug 8, 2024
View reviewed changes
Copy link
Contributor

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

Great start, thanks a lot for doing this!

@lmolkova lmolkova mentioned this pull request Aug 16, 2024
Merged
1 task
xrmx
xrmx reviewed Aug 19, 2024
View reviewed changes
...ation/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/patch.py Outdated Show resolved Hide resolved
...ation/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/utils.py Outdated Show resolved Hide resolved
...ation/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/utils.py Outdated Show resolved Hide resolved
...ation/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/utils.py Outdated Show resolved Hide resolved
...ation/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/utils.py Outdated Show resolved Hide resolved
...ation/opentelemetry-instrumentation-openai/src/opentelemetry/instrumentation/openai/utils.py Outdated Show resolved Hide resolved
lmolkova added a commit to open-telemetry/opentelemetry-specification that referenced this pull request Aug 28, 2024
@lmolkova @arminru
Update instrumentation library naming guidance to avoid conflicts bet…
6672dbc 
…ween external and otel instrumentations (#4187)

Some package managers (PyPi) don't provide means to reserve namespaces
for projects. We also have a number of **external** instrumentation
libraries in python that follow current guidance and use
`opentelemetry-instrumentation-{component}` naming pattern.

These libraries are hard to distinguish from otel-authored ones. Also,
when someone (legitimately following existing guidance) creates an
external instrumentation package like this, it blocks our ability to
have OTel-authored instrumentation with this 'good' name.

See
open-telemetry/opentelemetry-python-contrib#2759 (comment)
for real-life example.

## Changes

This PR changes the recommendation to:
- otel authored instrumentation should use
`opentelemetry-instrumentation-*` pattern
- external instrumentation should not use this pattern and should prefix
lib name with their company/project/etc name

* ~~[ ] Related issues #~~
* ~~[ ] Related [OTEP(s)](https://github.com/open-telemetry/oteps) #~~
* ~~[ ] Links to the prototypes (when adding or changing features)~~
* [x]
[`CHANGELOG.md`](https://github.com/open-telemetry/opentelemetry-specification/blob/main/CHANGELOG.md)
file updated for non-trivial changes
* ~~[ ]
[`spec-compliance-matrix.md`](https://github.com/open-telemetry/opentelemetry-specification/blob/main/spec-compliance-matrix.md)
updated if necessary~~

---------

Co-authored-by: Armin Ruech <[email protected]>
@lzchen lzchen added the gen-ai Related to generative AI label Aug 28, 2024
@lzchen
Copy link
Contributor

lzchen commented Oct 19, 2024

@alizenhom

actually TIL, that the sampler will lose access to the attributes if they are manually added after span creation, i see the point but this might need some additional work.

To my understanding it should be a fairly simple change. Is there a reason why we can't just pass in span_attributes into start_span?

@alizenhom
Copy link
Contributor

@lzchen addressed all of the above comments

lzchen
lzchen approved these changes Oct 21, 2024
View reviewed changes
Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

@karthikscale3 @alizenhom

Great job on this! LGTM

@karthikscale3
Copy link
Contributor Author

@karthikscale3 @alizenhom

Great job on this! LGTM

Thanks for the quick reviews @lzchen !

@karthikscale3 karthikscale3 reopened this Oct 21, 2024
@karthikscale3
Copy link
Contributor Author

karthikscale3 commented Oct 22, 2024 •
edited
Loading

@lzchen / @xrmx - Thanks again for all your reviews. If everything looks good, do you mind merging the PR? We don't seem to have the access to merge it.

@lzchen lzchen merged commit beed0aa into open-telemetry:main Oct 22, 2024
535 checks passed
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@lmolkova @arminru
Update instrumentation library naming guidance to avoid conflicts bet…
cb8e044 
…ween external and otel instrumentations (open-telemetry#4187)

Some package managers (PyPi) don't provide means to reserve namespaces
for projects. We also have a number of **external** instrumentation
libraries in python that follow current guidance and use
`opentelemetry-instrumentation-{component}` naming pattern.

These libraries are hard to distinguish from otel-authored ones. Also,
when someone (legitimately following existing guidance) creates an
external instrumentation package like this, it blocks our ability to
have OTel-authored instrumentation with this 'good' name.

See
open-telemetry/opentelemetry-python-contrib#2759 (comment)
for real-life example.

## Changes

This PR changes the recommendation to:
- otel authored instrumentation should use
`opentelemetry-instrumentation-*` pattern
- external instrumentation should not use this pattern and should prefix
lib name with their company/project/etc name

* ~~[ ] Related issues #~~
* ~~[ ] Related [OTEP(s)](https://github.com/open-telemetry/oteps) #~~
* ~~[ ] Links to the prototypes (when adding or changing features)~~
* [x]
[`CHANGELOG.md`](https://github.com/open-telemetry/opentelemetry-specification/blob/main/CHANGELOG.md)
file updated for non-trivial changes
* ~~[ ]
[`spec-compliance-matrix.md`](https://github.com/open-telemetry/opentelemetry-specification/blob/main/spec-compliance-matrix.md)
updated if necessary~~

---------

Co-authored-by: Armin Ruech <[email protected]>
@dasiths
Copy link

dasiths commented Nov 2, 2024

Thank for the contribution. When is this planned for release?

@karthikscale3
Copy link
Contributor Author

Thank for the contribution. When is this planned for release?

@dasiths - We are planning to release it soon (likely later this week). cc @lmolkova

@jsburckhardt
Copy link

Thank for the contribution. When is this planned for release?

@dasiths - We are planning to release it soon (likely later this week). cc @lmolkova

Saw it got released yesterday in v0.49b0 - but still not in pypi :(

@lmolkova
Copy link
Contributor

lmolkova commented Nov 7, 2024

it will be released separately (now and in the future), release is still in progress, got stuck on some minor issues. Hope to finish tomorrow.

@karthikscale3
Copy link
Contributor Author

it will be released separately (now and in the future), release is still in progress, got stuck on some minor issues. Hope to finish tomorrow.

Thanks @lmolkova

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alizenhom alizenhom alizenhom left review comments

@lmolkova lmolkova lmolkova approved these changes

@lzchen lzchen lzchen approved these changes

@codefromthecrypt codefromthecrypt codefromthecrypt approved these changes

@xrmx xrmx xrmx approved these changes

Assignees
No one assigned
Labels
gen-ai Related to generative AI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Contributing an OpenAI instrumentor.
9 participants
@karthikscale3 @truptiparkar7 @lmolkova @lzchen @alizenhom @dasiths @jsburckhardt @xrmx @codefromthecrypt