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

Add additional LLM span attributes #1059

Conversation

karthikscale3
Copy link
Contributor

@karthikscale3 karthikscale3 commented May 22, 2024

Fixes #

Changes

Please provide a brief description of the changes here.

Adding additional attributes to LLM span attributes.

Note: if the PR is touching an area that is not listed in the existing areas, or the area does not have sufficient domain experts coverage, the PR might be tagged as experts needed and move slowly until experts are identified.

Merge requirement checklist

@karthikscale3 karthikscale3 requested review from a team May 22, 2024 15:09
Copy link

linux-foundation-easycla bot commented May 22, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

model/registry/gen-ai.yaml Outdated Show resolved Hide resolved
model/registry/gen-ai.yaml Outdated Show resolved Hide resolved
model/registry/gen-ai.yaml Outdated Show resolved Hide resolved
model/registry/gen-ai.yaml Outdated Show resolved Hide resolved
.chloggen/839.yaml Outdated Show resolved Hide resolved
@karthikscale3
Copy link
Contributor Author

thanks @lmolkova . I have addressed all your review comments.

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.

yaml changes look good, but please make sure to regenerate markdown tables

docs/attributes-registry/gen-ai.md Outdated Show resolved Hide resolved
docs/attributes-registry/gen-ai.md Outdated Show resolved Hide resolved
@drewby drewby self-requested a review June 13, 2024 06:36
Copy link
Member

@drewby drewby left a comment

Choose a reason for hiding this comment

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

@karthikscale3, can you resolve the conversations (including two changes from Liudmila and we can approve and merge)

model/trace/gen-ai.yaml Outdated Show resolved Hide resolved
@karthikscale3
Copy link
Contributor Author

@karthikscale3, can you resolve the conversations (including two changes from Liudmila and we can approve and merge)

Apologies for the delay. Updated the PR

  • incorporated feedbacks and resolved comments
  • regenerated markdowns

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.

Left some cosmetic comment, LGTM otherwise

model/registry/gen-ai.yaml Outdated Show resolved Hide resolved
model/registry/gen-ai.yaml Outdated Show resolved Hide resolved
model/registry/gen-ai.yaml Outdated Show resolved Hide resolved
model/registry/gen-ai.yaml Outdated Show resolved Hide resolved
model/registry/gen-ai.yaml Outdated Show resolved Hide resolved
@karthikscale3
Copy link
Contributor Author

Left some cosmetic comment, LGTM otherwise

Thanks @lmolkova! I have updated the PR again. Looking for one more approval and then we should be good to merge this one. cc @drewby

Copy link
Contributor

@nirga nirga left a comment

Choose a reason for hiding this comment

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

@lmolkova I think this is good to go

@nirga
Copy link
Contributor

nirga commented Jun 19, 2024

Oh wait @karthikscale3 there’s some failures in CI

@lmolkova
Copy link
Contributor

@karthikscale3 I think you have to regenerate md (make attribute-registry-generation table-generation )

@karthikscale3
Copy link
Contributor Author

make attribute-registry-generation table-generation

Thank you! Regenerated and pushed it back up. Will monitor for any other failures

@lmolkova lmolkova merged commit 82c0926 into open-telemetry:main Jun 20, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

7 participants