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

Update UseOpenTelemetry for latest genai spec updates #5532

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Oct 17, 2024

  • Events are now expected to be emitted as body fields, and the newly-recommended way to achieve that is via ILogger. So UseOpenTelemetry now takes an optional logger that it uses for emitting such data.
  • I restructured the implementation to reduce duplication.
  • Added logging of response format and seed.
  • Added ChatOptions.TopK, as it's one of the parameters considered special by the spec.
  • Updated the Azure.AI.Inference provider name to match the convention and what the library itself uses
  • Updated the OpenAI client to use openai regardless of the kind of the actual client being used, per spec and recommendation

Closes #5523

Microsoft Reviewers: Open in CodeFlow

@tarekgh
Copy link
Member

tarekgh commented Oct 17, 2024

    _meter = new(name);

I am wondering if OpenTelemetryChatClient will be created many times? I am asking because it creates ActivitySource, Logger, and Meter and dispose them when the object is not in use any more. If it is expected the component is used inside a container, would it be better to use IMeterFactory for instance like:

var meterFactory = services.GetRequiredService();
var meter = meterFactory.Create("HatCo.Store");


Refers to: src/Libraries/Microsoft.Extensions.AI/ChatCompletion/OpenTelemetryChatClient.cs:67 in 4594124. [](commit_id = 4594124, deletion_comment = False)

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

Reviewed the usage of the telemetry stuff and left a couple of questions. LGTM!

- Events are now expected to be emitted as body fields, and the newly-recommended way to achieve that is via ILogger. So UseOpenTelemetry now takes an optional logger that it uses for emitting such data.
- I restructured the implementation to reduce duplication.
- Added logging of response format and seed.
- Added ChatOptions.TopK, as it's one of the parameters considered special by the spec.
- Updated the Azure.AI.Inference provider name to match the convention and what the library itself uses
- Updated the OpenAI client to use openai regardless of the kind of the actual client being used, per spec and recommendation
@stephentoub
Copy link
Member Author

I am wondering if OpenTelemetryChatClient will be created many times? I am asking because it creates ActivitySource, Logger, and Meter and dispose them when the object is not in use any more. If it is expected the component is used inside a container, would it be better to use IMeterFactory for instance like:

It might be; it'll end up typically depending on how instances are registered in DI, whether as singleton or scoped.

It could take an IMeterFactory, but that'd require adding a ctor parameter, and I don't see us doing that in many places. Is it recommended? It also wouldn't help with the ActivitySource.

@stephentoub stephentoub merged commit 8690e7a into dotnet:main Oct 17, 2024
6 checks passed
@stephentoub stephentoub deleted the updateotel branch October 17, 2024 20:55
@github-actions github-actions bot locked and limited conversation to collaborators Nov 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update UseOpenTelemetry for latest semantic convention
3 participants