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 open telemetry to Azure.AI.Inferencing #45751

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

Add open telemetry to Azure.AI.Inferencing #45751

wants to merge 30 commits into from
+2,469 −4,031

Conversation

nick863
Copy link

@nick863 nick863 commented Aug 30, 2024 •
edited
Loading

In this PR we are adding the Open Telemetry in Azure.AI.Inferencing.

@nick863 nick863 changed the title Draft Add open telemetry [Draft] Add open telemetry Aug 30, 2024
@azure-sdk
Copy link
Collaborator

azure-sdk commented Aug 30, 2024 •
edited
Loading

API change check

APIView has identified API level changes in this PR and created following API reviews.

Azure.AI.Inference

jsquire
jsquire requested changes Sep 10, 2024
View reviewed changes
....Inference/tests/SessionRecords/InferenceClientTelemetryTest/TestBadChatResponse(Plane).json Outdated Show resolved Hide resolved
sdk/ai/Azure.AI.Inference/tests/InferenceClientTelemetryTest.cs Outdated Show resolved Hide resolved
sdk/ai/Azure.AI.Inference/tests/Utilities/ValidatingActivityListener.cs Outdated Show resolved Hide resolved
sdk/ai/Azure.AI.Inference/samples/Sample8_ChatCompletionsWithTelemetry.md Outdated Show resolved Hide resolved
sdk/ai/Azure.AI.Inference/samples/Sample8_ChatCompletionsWithTelemetry.md Outdated Show resolved Hide resolved
sdk/ai/Azure.AI.Inference/samples/Sample8_ChatCompletionsWithTelemetry.md Outdated Show resolved Hide resolved
sdk/ai/Azure.AI.Inference/samples/Sample8_ChatCompletionsWithTelemetry.md Outdated
const string ACTIVITY = "Azure.AI.Inference.ChatCompletionsClient";
```

To log metrics and events to Application Insights we will need to get the connection string. Please open the Azure portal, create the Application Insights resource you wish to use for storing the telemetry. After that open the main page and find the "Connection String". It generally will have the format similar to "InstrumentationKey=xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx;IngestionEndpoint=https://region-x.in.applicationinsights.azure.com/;LiveEndpoint=https://eastus.livediagnostics.monitor.azure.com/;ApplicationId=xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx". In our code we will create the constant, storing the connection string.
Copy link
Member

Choose a reason for hiding this comment

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

@scottaddie : Should we be adding new samples that are based on connection strings, or should be demonstrating identity credentials for all the things?

sdk/ai/Azure.AI.Inference/samples/Sample8_ChatCompletionsWithTelemetry.md Outdated Show resolved Hide resolved
sdk/ai/Azure.AI.Inference/src/Telemetry/AbstractRecordedResponse.cs Outdated Show resolved Hide resolved
eng/Packages.Data.props
@@ -391,6 +391,13 @@
<PackageReference Update="Microsoft.Extensions.Logging.Configuration" Version="2.1.1" />
</ItemGroup>

<ItemGroup Condition="$(MSBuildProjectName.StartsWith('Azure.AI.Inference.Tests'))">
Copy link
Member

Choose a reason for hiding this comment

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

There's no justification for isolating this to just the AI.Inference library. We'll either want to approve as a general dependency for tests/samples or not.

@KrzysztofCwalina, @tg-msft : I'd appreciate your thoughts.

@jsquire
Copy link
Member

jsquire commented Sep 10, 2024

@lmolkova: If you would be so kind as to take a look at the OTel bits, it would be appreciated.

@dargilco
Copy link
Member

Does the NuGet now declare a new dependency on one or more tracing packages, even if I don't want to turn on tracing? If so, what is the total size of these additional packages?

@nick863
Copy link
Author

nick863 commented Sep 11, 2024

Does the NuGet now declare a new dependency on one or more tracing packages, even if I don't want to turn on tracing? If so, what is the total size of these additional packages?

The Open Telemetry is not a dependency of Azure.AI.Inference. In this PR we create an Activity and if user created a Listener from scratch (as we did in E2E tests) or the Tracer and Meter providers, it will log the metrics and send it to listener. This part is done by the standard .NET library System.Diagnostics. The open telemetry is in the user space, user will need to install the appropriate dependencies separately.
We only add the OpenTelemetry as a test dependency, only in test purposes.

@lmolkova lmolkova changed the title Add open elemetry to Azure.AI.Inferencing Add open telemetry to Azure.AI.Inferencing Sep 11, 2024
lmolkova
lmolkova reviewed Sep 11, 2024
View reviewed changes
sdk/ai/Azure.AI.Inference/src/Customized/ChatCompletionsClient.cs Outdated Show resolved Hide resolved
sdk/ai/Azure.AI.Inference/src/Customized/ChatCompletionsClient.cs Outdated Show resolved Hide resolved
sdk/ai/Azure.AI.Inference/src/Customized/ChatCompletionsClient.cs Outdated Show resolved Hide resolved
sdk/ai/Azure.AI.Inference/src/Customized/ChatCompletionsClient.cs Show resolved Hide resolved
sdk/ai/Azure.AI.Inference/src/Helpers/SseAsyncEnumerator.cs Show resolved Hide resolved
sdk/ai/Azure.AI.Inference/src/Telemetry/OpenTelemetryScope.cs Outdated Show resolved Hide resolved
sdk/ai/Azure.AI.Inference/src/Telemetry/OpenTelemetryScope.cs Outdated Show resolved Hide resolved
sdk/ai/Azure.AI.Inference/src/Telemetry/OpenTelemetryScope.cs Outdated Show resolved Hide resolved
sdk/ai/Azure.AI.Inference/src/Telemetry/OpenTelemetryScope.cs Outdated Show resolved Hide resolved
sdk/ai/Azure.AI.Inference/src/Telemetry/OpenTelemetryScope.cs Outdated Show resolved Hide resolved
@nick863 nick863 requested a review from a team as a code owner September 19, 2024 02:50
glharper
glharper reviewed Sep 21, 2024
View reviewed changes
sdk/ai/Azure.AI.Inference/src/Telemetry/OpenTelemetryScope.cs
return "";
}

public static bool GetSwithVariableVal(string name)
Copy link
Member

Choose a reason for hiding this comment

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

spelling

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

@lmolkova lmolkova lmolkova left review comments

@annelo-msft annelo-msft annelo-msft left review comments

@dargilco dargilco dargilco left review comments

@glharper glharper glharper left review comments

@howieleung howieleung howieleung left review comments

@jsquire jsquire jsquire requested changes

@jhakulin jhakulin Awaiting requested review from jhakulin jhakulin is a code owner

@JoshLove-msft JoshLove-msft Awaiting requested review from JoshLove-msft JoshLove-msft is a code owner

@christothes christothes Awaiting requested review from christothes christothes is a code owner

@KrzysztofCwalina KrzysztofCwalina Awaiting requested review from KrzysztofCwalina KrzysztofCwalina is a code owner

@tg-msft tg-msft Awaiting requested review from tg-msft tg-msft is a code owner

@m-nash m-nash Awaiting requested review from m-nash m-nash is a code owner

@ArthurMa1978 ArthurMa1978 Awaiting requested review from ArthurMa1978 ArthurMa1978 is a code owner

@trangevi trangevi Awaiting requested review from trangevi trangevi is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
AI Model Inference
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@nick863 @azure-sdk @jsquire @dargilco @lmolkova @annelo-msft @trangevi @glharper @howieleung