Skip to content

Conversation

@keith-decker
Copy link
Contributor

Description

Add missing SemConv attributes to spans for LLMInvocation calls, clean up some repeated code in the unit tests for genai utils.

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?

Updated unit tests to check for additional attributes

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

Copy link

@zhirafovod zhirafovod left a comment

Choose a reason for hiding this comment

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

2 NIT comments, but looks good otherwise

_maybe_set_span_messages(
span, invocation.input_messages, invocation.output_messages
)
_apply_request_attributes(span, invocation)
Copy link
Member

Choose a reason for hiding this comment

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

not blocking for this PR, but we should eventually allow to set attributes at start time. I noticed we don't properly mark which attributes should be provided at the start time, fixing it in the open-telemetry/semantic-conventions#2994

else:
finish_reasons = None

if finish_reasons:
Copy link
Member

Choose a reason for hiding this comment

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

I believe if finish_reasons is not available, we should set error - open-telemetry/semantic-conventions#2919 (comment)


from dataclasses import asdict
from typing import List
from typing import Any, Dict, List, Optional
Copy link
Member

Choose a reason for hiding this comment

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

nit prefer to use dict[k, v], and Foo | None with from __future__ import annotations.

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.

5 participants