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

adding inference trace injection #36890

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

adding inference trace injection #36890

wants to merge 43 commits into from

Conversation

M-Hietala
Copy link
Contributor

@M-Hietala M-Hietala commented Aug 14, 2024 •
edited
Loading

Description

Adding a library that injects traces to azure-ai-inferencing.

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@dargilco
Copy link
Member

Is there a need to log some HTTP request and response headers? I see you only log request and response body. Also, do we log full connection url?

@M-Hietala
Copy link
Contributor Author

Is there a need to log some HTTP request and response headers? I see you only log request and response body. Also, do we log full connection url?

These are not logged as part of the GenAI semantic conventions that are added as part of this change. Connection URI and other information that looks to be http header related seem to get traced from elsewhere.

@azure-sdk
Copy link
Collaborator

azure-sdk commented Aug 16, 2024 •
edited
Loading

API change check

API changes are not detected in this pull request.

@howieleung howieleung mentioned this pull request Aug 22, 2024
Closed
3 tasks
lmolkova
lmolkova previously approved these changes Aug 23, 2024
View reviewed changes
Copy link
Member

@lmolkova lmolkova left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you!!

Would it be possible to try it out and attach some screenshots?

[update] nevermind, found your PR here https://github.com/Azure/azureml_run_specification/pull/1182

sdk/core/azure-core/azure/core/tracing/_inference_api_instrumentor_impl.py Outdated Show resolved Hide resolved
sdk/core/azure-core/azure/core/tracing/_inference_api_instrumentor_impl.py Outdated Show resolved Hide resolved
sdk/core/azure-core/azure/core/tracing/_inference_api_instrumentor_impl.py Outdated Show resolved Hide resolved
sdk/core/azure-core/azure/core/tracing/_inference_api_instrumentor_impl.py Outdated Show resolved Hide resolved
sdk/core/azure-core/azure/core/tracing/_inference_api_instrumentor_impl.py Outdated Show resolved Hide resolved
sdk/core/azure-core/azure/core/tracing/_inference_api_instrumentor_impl.py Outdated Show resolved Hide resolved
sdk/core/azure-core/azure/core/tracing/_inference_api_instrumentor_impl.py Outdated Show resolved Hide resolved
sdk/core/azure-core/azure/core/tracing/_inference_api_instrumentor_impl.py Outdated Show resolved Hide resolved
sdk/core/azure-core/azure/core/tracing/_inference_api_instrumentor_impl.py Outdated Show resolved Hide resolved
sdk/core/azure-core/azure/core/tracing/_ai_inference_api_instrumentor.py Outdated Show resolved Hide resolved
@lmolkova
Copy link
Member

would it be possible to add some tests (at least happy cases for chat with and without streaming, 1-2 failure cases)? They should probably live in the Azure AI inference itself

lmolkova
lmolkova reviewed Aug 23, 2024
View reviewed changes
Copy link
Member

@lmolkova lmolkova left a comment •
edited
Loading

Choose a reason for hiding this comment

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

[deleted]

@lmolkova lmolkova dismissed their stale review August 23, 2024 04:42

approved by mistake

@lmolkova lmolkova mentioned this pull request Aug 29, 2024
Open
@lmolkova lmolkova mentioned this pull request Sep 5, 2024
Merged
2 tasks
@M-Hietala M-Hietala marked this pull request as ready for review September 11, 2024 18:37
Marko Hietala and others added 19 commits September 26, 2024 14:00
fixing pre python v3.10 issue
521f7f0
@M-Hietala
Merge branch 'Azure:main' into mhietala/inference_genai_tracing
814f87f
test fixes
8c80099
@dargilco
Fix some of the non-trace tests
514dea4
fixing issues reported by tools
83f85d6
Merge branch 'mhietala/inference_genai_tracing' of https://github.com…
79ea9b3 
…/M-Hietala/azure-sdk-for-python into mhietala/inference_genai_tracing
adding uninstrumentation to the beginning of tracing tests
e8dd67d
updating readme and sample
0c286c3
adding ignore related to tool issue
1aaf87c
@M-Hietala
Merge branch 'Azure:main' into mhietala/inference_genai_tracing
a1b1f13
updating code snippet in readme
510a6ca
Merge branch 'mhietala/inference_genai_tracing' of https://github.com…
04da0e6 
…/M-Hietala/azure-sdk-for-python into mhietala/inference_genai_tracing

commits were rejected
@dargilco
Add missing @recorded_by_proxy decorators to new tracing tests
fa8e8b0
@dargilco
Push new recordings
e410c31
fixing issues reported by tools
18b3d92
Merge branch 'mhietala/inference_genai_tracing' of https://github.com…
200ab61 
…/M-Hietala/azure-sdk-for-python into mhietala/inference_genai_tracing

push rejected
adding inference to shared requirements
4a56354
@M-Hietala
Merge branch 'Azure:main' into mhietala/inference_genai_tracing
3113e35
remove inference from setup
58a754f
@scbedd
Copy link
Member

scbedd commented Oct 3, 2024

/check-enforcer override

kristapratico
kristapratico reviewed Oct 4, 2024
View reviewed changes
sdk/ai/azure-ai-inference/samples/sample_chat_completions_with_tracing.py Outdated Show resolved Hide resolved
...ure-core-tracing-opentelemetry/azure/core/tracing/ai/inference/_ai_inference_instrumentor.py Show resolved Hide resolved
sdk/ai/azure-ai-inference/azure/ai/inference/_patch.py Outdated Show resolved Hide resolved
...ore-tracing-opentelemetry/azure/core/tracing/ai/inference/_ai_inference_instrumentor_impl.py Show resolved Hide resolved
...ore-tracing-opentelemetry/azure/core/tracing/ai/inference/_ai_inference_instrumentor_impl.py Show resolved Hide resolved
...ore-tracing-opentelemetry/azure/core/tracing/ai/inference/_ai_inference_instrumentor_impl.py Outdated Show resolved Hide resolved
...ore-tracing-opentelemetry/azure/core/tracing/ai/inference/_ai_inference_instrumentor_impl.py Show resolved Hide resolved
shared_requirements.txt Show resolved Hide resolved
...ore-tracing-opentelemetry/azure/core/tracing/ai/inference/_ai_inference_instrumentor_impl.py Show resolved Hide resolved
...ore-tracing-opentelemetry/azure/core/tracing/ai/inference/_ai_inference_instrumentor_impl.py Show resolved Hide resolved
@lmolkova lmolkova mentioned this pull request Oct 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pvaneck pvaneck pvaneck left review comments

@lmolkova lmolkova lmolkova approved these changes

@johanste johanste johanste left review comments

@jhakulin jhakulin jhakulin left review comments

@nick863 nick863 nick863 left review comments

@kristapratico kristapratico kristapratico left review comments

@dargilco dargilco dargilco approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@M-Hietala @dargilco @azure-sdk @lmolkova @scbedd @pvaneck @johanste @jhakulin @nick863 @kristapratico