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

feat(SageMaker): Add SageMaker instrumentation #2028

Merged
merged 14 commits into from
Oct 2, 2024

Conversation

bobbywlindsey
Copy link
Contributor

@bobbywlindsey bobbywlindsey commented Sep 25, 2024

  • I have added tests that cover my changes.
  • If adding a new instrumentation or changing an existing one, I've added screenshots from some observability platform showing the change.
    sagemaker-instrumentation-cloudwatch
  • PR name follows conventional commits format: feat(instrumentation): ... or fix(instrumentation): ....
  • (If applicable) I have updated the documentation accordingly.

Summary of changes (closes #1660):

  • Added new instrumentation for Amazon SageMaker endpoints
  • Added 2 new semantic conventions:
    1. llm.sagemaker.request which captures the request body of the endpoint invocation
    2. llm.sagemaker.response which captures the response body of the endpoint invocation

@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Sep 25, 2024
@CLAassistant
Copy link

CLAassistant commented Sep 25, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@nirga nirga left a comment

Choose a reason for hiding this comment

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

Thanks so much @bobbywlindsey! Looking good :)
Left a couple of small comments

@@ -0,0 +1,33 @@
# OpenTelemetry Bedrock Instrumentation

<a href="https://pypi.org/project/opentelemetry-instrumentation-bedrock/">
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
<a href="https://pypi.org/project/opentelemetry-instrumentation-bedrock/">
<a href="https://pypi.org/project/opentelemetry-instrumentation-sagemaker/">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah totally missed this file, thanks for the catch. Will update.

from opentelemetry.instrumentation.sagemaker.streaming_wrapper import StreamingWrapper
from opentelemetry.instrumentation.sagemaker.utils import dont_throw
from wrapt import wrap_function_wrapper
import anthropic
Copy link
Member

Choose a reason for hiding this comment

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

do you need this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah nope, I'll remove it 👍🏻


_set_span_attribute(span, SpanAttributes.LLM_REQUEST_MODEL, endpoint_name)
_set_span_attribute(
span, SpanAttributes.LLM_SAGEMAKER_REQUEST, json.dumps(request_body)
Copy link
Member

Choose a reason for hiding this comment

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

I think I might use our "input" and "output" attributes instead:

(these will anyway be refactored to be logs in the near future as per decisions we're making at the otel semantic conventions WG)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM, will make the updates.

opentelemetry-instrumentation = "^0.48b0"
opentelemetry-semantic-conventions = "^0.48b0"
opentelemetry-semantic-conventions-ai = "0.4.1"
anthropic = ">=0.17.0"
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't be a dependency of the package

@nirga
Copy link
Member

nirga commented Sep 25, 2024

@bobbywlindsey looks like you need to run poetry lock within your package (see failing CI)

@nirga
Copy link
Member

nirga commented Sep 26, 2024

@bobbywlindsey seems like tests are failing - I think you'll need to amend the cassettes or the mock API URL you're using

@bobbywlindsey
Copy link
Contributor Author

@nirga Thanks for the help :) I think I've resolved the build issues now. Let me know if there's anything else I can do to improve the PR. Once this one is merged, I'll begin working on Bedrock Knowledge Bases and Agents issues 👍🏻

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Oct 2, 2024
@nirga nirga merged commit 346d752 into traceloop:main Oct 2, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer new instrumentation python Pull requests that update Python code size:XL This PR changes 500-999 lines, ignoring generated files. testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 Feature: Tracing support for Amazon SageMaker endpoints
3 participants