-
Notifications
You must be signed in to change notification settings - Fork 11
feat: Adding distributed tracing to decorator #403
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
base: main
Are you sure you want to change the base?
Conversation
| return (*key, project, log_stream) | ||
| # For distributed tracing, include trace_id/span_id in the cache key | ||
| # This allows nested calls within the same request to reuse the same logger instance | ||
| if trace_id or span_id: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert here that mode is streaming
| # Compute the key based on provided parameters or environment variables. | ||
| key = GalileoLoggerSingleton._get_key(project, log_stream, experiment_id, mode) | ||
| # If trace_id or span_id is provided, use streaming mode | ||
| if trace_id or span_id: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trace id and span id may not always both be provided right? also assert here that the mdoe is streaming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think u should have generic checks everywhere on mode + trace_id/span_id .. extract common helper func
| """ | ||
| # Return a shallow copy of the loggers dictionary to prevent external modifications. | ||
| return dict(self._galileo_loggers) | ||
| return self._galileo_loggers.copy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idgt code change
|
|
||
| _logger = logging.getLogger(__name__) | ||
|
|
||
| # Try to import middleware context (if middleware is being used) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need try/except for ur own library.. why would u not be useing middleware? isn't it a local library...
| return None, None | ||
|
|
||
|
|
||
| def _extract_from_flask_request(request_obj: Any) -> tuple[Optional[str], Optional[str]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this with fastapi/ flask? why are they different? also langsmith isn't doing flask the same way because something about WSGI vs ASGI...
but they are doing this for flask
# server.py
import langsmith as ls
from fastapi import FastAPI, Request
@ls.traceable
async def my_application():
...
app = FastAPI() # Or Flask, Django, or any other framework
@app.post("/my-route")
async def fake_route(request: Request):
# request.headers: {"langsmith-trace": "..."}
# as well as optional metadata/tags in `baggage`
with ls.tracing_context(parent=request.headers):
return await my_application()
OR
@app.post("/my-route")
async def fake_route(request: Request):
# request.headers: {"langsmith-trace": "..."}
my_application(langsmith_extra={"parent": request.headers})
| app.add_middleware(TracingMiddleware) | ||
| """ | ||
|
|
||
| async def dispatch(self, request: Request, call_next: Callable[[Request], Awaitable[Response]]) -> Response: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's happenign in dispatch and what's happening in the other distributed_tracing.py
| """ | ||
| self.get_logger_instance().set_session(session_id) | ||
|
|
||
| def get_tracing_headers(self) -> dict[str, str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review this file after
39dc164 to
8928f4b
Compare
| from contextvars import ContextVar | ||
| from typing import Optional | ||
|
|
||
| from starlette.middleware.base import BaseHTTPMiddleware |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Failed check: Test / test (ubuntu-latest, 3.9)
I’ve attached the relevant part of the log for your convenience:
ModuleNotFoundError: No module named 'starlette'. The import chain shows: galileo.decorator imports from galileo.utils.distributed_tracing, which imports from galileo.middleware.tracing, which attempts to import from starlette.middleware.base but the starlette module is not installed.
Finding type: Log Error
8681aab to
72b3c90
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #403 +/- ##
==========================================
- Coverage 86.22% 86.21% -0.02%
==========================================
Files 75 78 +3
Lines 5887 5976 +89
==========================================
+ Hits 5076 5152 +76
- Misses 811 824 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
72b3c90 to
bb99451
Compare
bb99451 to
eb6976a
Compare
User description
Shortcut:
Description:
Tests:
Generated description
Below is a concise technical summary of the changes proposed in this PR:
graph LR TracingMiddleware_dispatch_("TracingMiddleware.dispatch"):::added get_trace_id_("get_trace_id"):::added get_span_id_("get_span_id"):::added extract_tracing_headers_("extract_tracing_headers"):::added GalileoDecorator_prepare_call_("GalileoDecorator._prepare_call"):::modified GalileoLoggerSingleton_get_("GalileoLoggerSingleton.get"):::modified GalileoDecorator_get_logger_instance_("GalileoDecorator.get_logger_instance"):::modified validate_distributed_tracing_mode_("_validate_distributed_tracing_mode"):::added GalileoLoggerSingleton_get_key_("GalileoLoggerSingleton._get_key"):::modified TracingMiddleware_dispatch_ -- "Stores X-Trace-ID in context for distributed tracing" --> get_trace_id_ TracingMiddleware_dispatch_ -- "Stores X-Span-ID in context for distributed tracing" --> get_span_id_ extract_tracing_headers_ -- "Retrieves trace ID from context variables for tracing" --> get_trace_id_ extract_tracing_headers_ -- "Retrieves span ID from context variables for tracing" --> get_span_id_ GalileoDecorator_prepare_call_ -- "Extracts trace/span IDs from context or function arguments" --> extract_tracing_headers_ GalileoDecorator_prepare_call_ -- "Gets logger instance with trace/span IDs for tracing" --> GalileoLoggerSingleton_get_ GalileoDecorator_get_logger_instance_ -- "Retrieves logger with distributed tracing identifiers" --> GalileoLoggerSingleton_get_ GalileoLoggerSingleton_get_ -- "Validates distributed tracing mode is streaming" --> validate_distributed_tracing_mode_ GalileoLoggerSingleton_get_key_ -- "Ensures mode correctness when caching with trace/span IDs" --> validate_distributed_tracing_mode_ classDef added stroke:#15AA7A classDef removed stroke:#CD5270 classDef modified stroke:#EDAC4C linkStyle default stroke:#CBD5E1,font-size:13pxIntegrate distributed tracing capabilities into the
galileo-pythonmodule, enabling theGalileoDecoratorto automatically extract and propagate trace and span IDs across function calls. Introduce aTracingMiddlewarefor Starlette/FastAPI applications to seamlessly capture tracing headers from incoming HTTP requests, enhancing observability for distributed web services.TracingMiddlewarefor Starlette/FastAPI applications to automatically extractX-Trace-IDandX-Span-IDheaders from incoming HTTP requests. Store these IDs inContextVarobjects, making them accessible to theGalileoDecoratorthroughout the request lifecycle, thereby enabling automatic distributed tracing for web endpoints.Modified files (2)
Latest Contributors(0)
@logdecorator correctly utilizes distributed tracing IDs.Modified files (1)
Latest Contributors(0)
starletteas an optional dependency topyproject.tomland updatepoetry.lockto reflect this new dependency. This ensures that theTracingMiddlewarecan be used by applications built with Starlette or FastAPI.Modified files (2)
Latest Contributors(2)
GalileoDecoratorandGalileoLoggerSingleton. This involves modifying_prepare_calland_handle_call_resultto extract trace/span IDs from function arguments or context, passing them toget_logger_instance, and updating the logger singleton's key generation to include these IDs for proper caching and trace loading. Also, add aget_tracing_headersmethod toGalileoDecoratorfor manual header propagation.Modified files (3)
Latest Contributors(2)