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

Adding OpenTelemetry OTLP exporter support #54

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

Conversation

dasiths
Copy link
Member

@dasiths dasiths commented Sep 17, 2024

This PR adds the ability to export telemetry to an OTLP endpoint and has an example of collection telemetry locally.

It also includes an example of how to collect telemetry locally using the grafana/otel-lgtm container.

image

image

Copy link
Collaborator

@stuartleeks stuartleeks left a comment

Choose a reason for hiding this comment

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

This is looking good - thanks 😄

I will give it a proper test tomorrow but have left a couple of comments/questions

sample.env Outdated Show resolved Hide resolved
src/aoai-api-simulator/src/aoai_api_simulator/main.py Outdated Show resolved Hide resolved
@dasiths dasiths requested a review from stuartleeks September 25, 2024 08:34
@dasiths
Copy link
Member Author

dasiths commented Oct 7, 2024

Any update on this @stuartleeks

@stuartleeks
Copy link
Collaborator

Thanks for you patience @dasiths!

There are a couple of linter errors that look like they've been introduced in this PR but other than that this looks good to me: https://github.com/microsoft/aoai-api-simulator/actions/runs/11256643932?pr=54#summary-31299020880

For the "too many arguments" errors, I'd recommend suppressing the error (see the other methods in that same file). The other errors look like things that should be resolved.

@stuartleeks
Copy link
Collaborator

@dasiths I've pushed changes to resolve the conflict on latest main and address the quick-fix linter warnings. There are a couple remaining that I'm less clear on the path forwards. I've left a comment on the changed lines to make it easy to find - would love your help in understanding what the lines are doing :-)

@dasiths
Copy link
Member Author

dasiths commented Oct 17, 2024

@dasiths I've pushed changes to resolve the conflict on latest main and address the quick-fix linter warnings. There are a couple remaining that I'm less clear on the path forwards. I've left a comment on the changed lines to make it easy to find - would love your help in understanding what the lines are doing :-)

I replied to your comment up top. We may need to supress this linter error. :)

@dasiths
Copy link
Member Author

dasiths commented Nov 18, 2024

@stuartleeks is there anything required to get this landed? Sorry if I missed anything.

@stuartleeks
Copy link
Collaborator

@dasiths - I wanted to look at a slightly different way to handle the different conditions to see if it is possible to remove the private access. I started looking at it but and wanted to test the original code but wasn't able to get the exported metrics showing up locally. Sadly I didn't have much time to spend on it at that point. I do want to come back to it as I'd still like to get this working 😄

@dasiths
Copy link
Member Author

dasiths commented Dec 4, 2024

@dasiths - I wanted to look at a slightly different way to handle the different conditions to see if it is possible to remove the private access. I started looking at it but and wanted to test the original code but wasn't able to get the exported metrics showing up locally. Sadly I didn't have much time to spend on it at that point. I do want to come back to it as I'd still like to get this working 😄

Can confirm the metrics work but they take a bit of time to show up. It requires doing something like a small load test to really see the metrics light up.

@stuartleeks
Copy link
Collaborator

stuartleeks commented Dec 5, 2024

@dasiths I'm considering two potential alternatives currently.

The first option is to remove the call to configure_azure_monitor and explicitly wire up the Azure exporters if the App Insights config is set. I believe this would provide access to the relevant objects and remove the need to use the private accessors.

The second option is to remove the Azure otel distro and just use the standard exporters. When running locally, this would use the grafana/otel-lgtm container to capture/view the telemetry. When deployed to Azure it would either use the open telemetry collector or the corresponding feature in container apps

The second option would make the code simpler but would mean that telemetry when running locally wouldn't be send to App Insights (unless we also run the collector locally). Do you see this as an issue or is your primary goal to have local telemetry?

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.

2 participants