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(smolagents): add entrypoint for use in opentelemetry-instrument #1276

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

codefromthecrypt
Copy link
Contributor

@codefromthecrypt codefromthecrypt commented Feb 7, 2025

I mentioned this to @mikeldking, that openinference can become automatically enabled with opentelemetry's zero code approach for python, by adding entrypoints into the pyproject.toml.

Specifically, after this, as long as you have the dependencies of your main code, plus openinference-instrumentation-smolagents, and env exported with standard variables, if you launch with opentelemetry-instrument, traces will magically appear.

opentelemetry-instrument python main.py

Before adding to all projects, I wanted to check with smolagents first, if this is ok with y'all. I would love to be able to showcase this with Elastic Distribution of OpenTelemetry (EDOT) and the diff makes it all perfectly invisible.

diff --git a/examples/python/openinference/smolagents/main.py b/examples/python/openinference/smolagents/main.py
index 7d54c87..697f7a5 100644
--- a/examples/python/openinference/smolagents/main.py
+++ b/examples/python/openinference/smolagents/main.py
@@ -1,11 +1,7 @@
 import os
 
-from openinference.instrumentation.smolagents import SmolagentsInstrumentor
 from smolagents import DuckDuckGoSearchTool, OpenAIServerModel, ToolCallingAgent
 
-SmolagentsInstrumentor().instrument()
-
-
 def main():
     model = OpenAIServerModel(model_id=os.getenv("CHAT_MODEL", "gpt-4o-mini"))
 
diff --git a/examples/python/openinference/smolagents/requirements.txt b/examples/python/openinference/smolagents/requirements.txt
index 8d8c23b..5220f5a 100644
--- a/examples/python/openinference/smolagents/requirements.txt
+++ b/examples/python/openinference/smolagents/requirements.txt
@@ -3,4 +3,5 @@ smolagents[openai]~=1.7.0
 opentelemetry-sdk~=1.30.0
 opentelemetry-exporter-otlp-proto-http~=1.30.0
 opentelemetry-distro~=0.51b0
-openinference-instrumentation-smolagents~=0.1.3
+# openinference-instrumentation-smolagents~=0.1.3
+-e /Users/adriancole/oss/openinference/python/instrumentation/openinference-instrumentation-smolagents

@codefromthecrypt codefromthecrypt requested a review from a team as a code owner February 7, 2025 02:31
@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Feb 7, 2025
Copy link
Contributor

github-actions bot commented Feb 7, 2025

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@codefromthecrypt
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Feb 7, 2025
Copy link
Contributor

@nate-mar nate-mar left a comment

Choose a reason for hiding this comment

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

Cool! TIL! Thanks for the contribution. lgtm!

@nate-mar nate-mar merged commit 7d2af53 into Arize-ai:main Feb 7, 2025
9 checks passed
@github-actions github-actions bot mentioned this pull request Feb 7, 2025
@codefromthecrypt
Copy link
Contributor Author

Just verified this works like a champ with latest release. Should I raise PRs for the others? If so, same PR for all others, or one each like this?

@codefromthecrypt codefromthecrypt deleted the entrypoints branch February 7, 2025 03:28
@mikeldking
Copy link
Contributor

Just verified this works like a champ with latest release. Should I raise PRs for the others? If so, same PR for all others, or one each like this

Thanks so much for this @codefromthecrypt can do them all at once no problem. We have a single release please pipeline now. Should just work

@codefromthecrypt
Copy link
Contributor Author

ok I may be somewhat blind now, but all the rest are done in #1278 ;)

@aymeric-roucher
Copy link
Contributor

@codefromthecrypt maintainer of smolagents here: could you explain what this changes for instrumentation, and if we have actions to take to make it work?

@nate-mar
Copy link
Contributor

nate-mar commented Feb 14, 2025

Hi @aymeric-roucher ! As far as I understand, this doesn't change anything with the existing instrumentation for smolagents. It just gives users more optionality for setting up smolagents instrumentation with their apps, by allowing them to remove some boilerplate otel instrumentation code (via a run time arg instead) , if they so desire.

@codefromthecrypt
Copy link
Contributor Author

@aymeric-roucher I concur with @nate-mar and was going to propose an instruction change PR today for smolagents repo. You can then assess that to see if you prefer the before or after, but the existing instructions still work!

@aymeric-roucher
Copy link
Contributor

aymeric-roucher commented Feb 15, 2025

Hi all!

Bear in mind neither I nor smolagents user don't really care about the low level detail. Our common objective is "be able to robustly instrument a smolagents agent in as few lines of code as possible, with the asymptotic objective of 0 lines of code". More than 1 line for instrumentation is probably too much (bear in mind neither me nor my users care about setting up the system "in the right way", we just want it to work), 0 would be nice (but you probably at least need to set the endpoint url).

So please assume my only goal is to reduce the lines of code needed to implement instrumentation compared to our explanation in the doc.

The previous recommended (because shortest) implementation is:

from opentelemetry import trace
from opentelemetry.sdk.trace import TracerProvider
from opentelemetry.sdk.trace.export import BatchSpanProcessor

from openinference.instrumentation.smolagents import SmolagentsInstrumentor
from opentelemetry.exporter.otlp.proto.http.trace_exporter import OTLPSpanExporter
from opentelemetry.sdk.trace.export import ConsoleSpanExporter, SimpleSpanProcessor

endpoint = "http://0.0.0.0:6006/v1/traces"
trace_provider = TracerProvider()
trace_provider.add_span_processor(SimpleSpanProcessor(OTLPSpanExporter(endpoint)))

SmolagentsInstrumentor().instrument(tracer_provider=trace_provider)

This is 12 lines.

What is the new shortest possible implementation that works?

@axiomofjoy
Copy link
Contributor

Hey @aymeric-roucher, thanks for the comment! Definitely understand the desire to encapsulate the complexity of using OTel. We have helper functions that will shorten the number of lines of code.

pip install arize-phoenix-otel openinference-instrumentation-smolagents smolagents

from phoenix.otel import register
from openinference.instrumentation.smolagents import SmolagentsInstrumentor

register()
SmolagentsInstrumentor().instrument()

The register function accepts arguments for endpoint and project_name, e.g., register(endpoint="http://127.0.0.1:6006/v1/traces", project_name="smolagents"). By default, traces are sent to localhost on the default Phoenix port for GRPC traces.

The goal of register is to provide users who are not familiar with OpenTelemetry with a simple first-touch experience.

The zero-code approach enabled by this PR is probably best-suited for users who have familiarity with OTel concepts and wish to express those concepts in configuration (e.g., env vars) rather than code.

Let us know if this addresses the pain point you are trying to resolve!

@codefromthecrypt
Copy link
Contributor Author

codefromthecrypt commented Feb 16, 2025

if you want to:

  • not change how python is run (e.g. not prefix it with opentelemetry-instrument)

And still get the "zero code affect"

You can add the following single line to your source instead

import opentelemetry.instrumentation.auto_instrumentation.sitecustomize

Note that this is undocumented in otel and I'm trying to change that, also there's an outstanding bug where if you don't have PYTHONPATH, you need to set it.

I'm not sure if from phoenix.otel import register does the standard ENV var part or not, so this may be the same thing.

Here's the issue in otel I'm trying to get polished into a more known state, as there are valid reasons to want to do "single line of code" vs "zero code" open-telemetry/opentelemetry-python-contrib#3263

Regardless, I think manual is the least desirable option for new folks as it is easy to get wrong, adds maintenance and also will miss things like metrics when they become in use. I know this is just my opinion.

@axiomofjoy
Copy link
Contributor

Thanks for the context @codefromthecrypt.

Regardless, I think manual is the least desirable option for new folks as it is easy to get wrong, adds maintenance and also will miss things like metrics when they become in use. I know this is just my opinion.

Agree.

I have opened an issue here for incorporating this behavior into phoenix.otel.register and making it truly one-line instrumentation.

@aymeric-roucher
Copy link
Contributor

Thank you @axiomofjoy @codefromthecrypt for your explanations! I'll start by incorporating the short initialization proposed by @axiomofjoy into the implementation.

@codefromthecrypt
Copy link
Contributor Author

Cool. Link any PR back here for those following. Cheers!

@aymeric-roucher
Copy link
Contributor

@axiomofjoy when I run phoenix serve in a console, then register() in my script, I got the register() correctly pointing to the localhost url that the phoenix serve was listening to. Is there a possibility that that's not the case? In that case how to point either register() or phoenix serve to the correct url?

@axiomofjoy
Copy link
Contributor

@axiomofjoy when I run phoenix serve in a console, then register() in my script, I got the register() correctly pointing to the localhost url that the phoenix serve was listening to. Is there a possibility that that's not the case? In that case how to point either register() or phoenix serve to the correct url?

Hey @aymeric-roucher 👋

By default, Phoenix will listen for HTTP traces on port 6006 and for gRPC traces on port 4317. By default, phoenix.otel.register will configure traces to be exported to the default gRPC port 4317 on localhost. So if a user launches Phoenix locally with the default ports, there should be no issue.

If a user has configured a non-default port or is running a remote instance of Phoenix, they will need to set the endpoint and protocol arguments of register. See here for details.

@axiomofjoy
Copy link
Contributor

@aymeric-roucher As of openinference-instrumentation-smolagents==0.1.6 and arize-phoenix-otel==0.8.0, one-line instrumentation is possible.

from phoenix.otel import register

register(auto_instrument=True)  # automatically instruments all installed OpenInference libraries

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants