feat(core): Enable simple instrumentation for Otel#183
feat(core): Enable simple instrumentation for Otel#183
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #183 +/- ##
==========================================
+ Coverage 79.68% 80.11% +0.43%
==========================================
Files 42 43 +1
Lines 2815 2876 +61
==========================================
+ Hits 2243 2304 +61
Misses 572 572 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Andrii Soldatenko <andrii.soldatenko@gmail.com>
Co-authored-by: Andrii Soldatenko <andrii.soldatenko@gmail.com>
franz101
force-pushed
the
otel_instrument
branch
from
8572ecb to
f74946a
Compare
|
@ajaynayak @setu4993 @andriisoldatenko are you free to review, I have the docs also added for it |
| log_stream: str | None = None, | ||
| session_id: str | UUID | None = None, | ||
| console_url: str = "", | ||
| auto_instrument: bool = True, |
There was a problem hiding this comment.
How much overhead does auto instrumentation of the libraries below add to a user's app? Should this be True by default?
There was a problem hiding this comment.
Whoops my comment was not posted:
Here is an Example
from openinference.instrumentation.langchain import LangChainInstrumentor
LangChainInstrumentor().instrument(tracer_provider=tracer_provider)
https://github.com/Focadecombate/docling-test/blob/main/src/otel.py
There was a problem hiding this comment.
quite complex if you don't know what a trace provider is
There was a problem hiding this comment.
@franz101 I'm referring to the code block below, since auto_instrument is set to True by default, these will always be instantiated and I'm not sure if this is necessary. Feels like the default should be False.
# (Optional) auto‑instrument popular Gen‑AI libs
if auto_instrument:
_safe_instrument("openinference.instrumentation.openai", "OpenAIInstrumentor")
_safe_instrument("openinference.instrumentation.litellm", "LiteLLMInstrumentor")
_safe_instrument("openinference.instrumentation.langchain", "LangchainInstrumentor")
_safe_instrument("openinference.instrumentation.crewai", "CrewAIInstrumentor")
_safe_instrument("openinference.instrumentation.google_genai", "GoogleGenAIInstrumentor")
_safe_instrument("openinference.instrumentation.vertexai", "VertexAIInstrumentor")
_safe_instrument("openinference.instrumentation.llama_index", "LlamaIndexInstrumentor")
There was a problem hiding this comment.
Putting the user first, I would disagree :
- Import will only haben if the instrumentation is installed
- If you just want to log, you don't want to know what an instrumentor is
- Making the barrier to entry as low as possible here
| otel = [ | ||
| "opentelemetry-api>=1.34.1,<2.0.0", | ||
| "opentelemetry-sdk>=1.30.0,<2.0.0", | ||
| "opentelemetry-exporter-otlp>=1.30.0,<2.0.0", | ||
| "openinference-instrumentation-openai>=0.1.30,<0.2.0", | ||
| "openinference-instrumentation-litellm>=0.1.22,<0.2.0", | ||
| "openinference-instrumentation-langchain>=0.1.43,<0.2.0", | ||
| ] |
There was a problem hiding this comment.
We shouldn't need all of these at all times? Anyone using OpenAI doesn't need litellm / langchain...
There was a problem hiding this comment.
reason was, these three are the most common one
we could split them up even?
galileo[otel-langchain]
| log_stream: str | None = None, | ||
| session_id: str | UUID | None = None, | ||
| console_url: str = "", | ||
| auto_instrument: bool = True, |
|
@ajaynayak @setu4993 I added my reasoning for making auto instrument true by default:
Let's make a decision here so we can merge it, if users complain we can still change it. |
Focadecombate
left a comment
Focadecombate
left a comment
There was a problem hiding this comment.
For me it's fine, auto-instrumentation is something that is pushed by otel so I don't think it should be an issue. So it's a good way of making the barrier of entry into Galileo lower.
5 participants
Simply import
from galileo.otel import enable_tracing