feat(ibis): add OpenTelemetry tracing logs #1080
Conversation
WalkthroughThe changes integrate OpenTelemetry tracing throughout the Ibis server. A new "Enable Tracing" section is added to the README with instructions and a new command target in the justfile. Instrumentation is applied to many functions across modules—including rewriter, model substitution, connector classes, and FastAPI endpoints—by wrapping them with OpenTelemetry tracer spans. Additionally, utility functions have been enhanced for header context management, and new dependencies for OpenTelemetry have been added to the project configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Tracer
participant Service
Client->>API: Sends request with headers
API->>Tracer: Start span (build_context extracts header info)
API->>Service: Execute business logic
Service-->>API: Return result
Tracer-->>API: End span
API->>Client: Return response
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 LanguageToolibis-server/README.md[uncategorized] ~81-~81: Possible missing preposition found. (AI_HYDRA_LEO_MISSING_TO) [grammar] ~82-~82: The pronoun ‘We’ must be used with a non-third-person form of a verb. (NON3PRS_VERB) ⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
ibis-server/app/routers/v2/connector.py (1)
114-115: 🛠️ Refactor suggestionMissing tracing for get_db_version endpoint
While all other endpoints have been updated to include tracing, the
get_db_versionendpoint has been overlooked.For consistency, consider adding tracing to this endpoint as well:
@router.post("/{data_source}/metadata/version") -def get_db_version(data_source: DataSource, dto: MetadataDTO) -> str: - return MetadataFactory.get_metadata(data_source, dto.connection_info).get_version() +def get_db_version( + data_source: DataSource, + dto: MetadataDTO, + headers: Annotated[str | None, Header()] = None, +) -> str: + span_name = f"v2_metadata_version_{data_source}" + with tracer.start_as_current_span( + name=span_name, kind=trace.SpanKind.SERVER, context=build_context(headers) + ): + return MetadataFactory.get_metadata(data_source, dto.connection_info).get_version()
🧹 Nitpick comments (10)
ibis-server/pyproject.toml (1)
39-40: Appropriate OpenTelemetry dependenciesAdding these dependencies with version constraints of
>=1.30.0is the correct approach. This allows compatibility with future versions while ensuring a minimum feature set.However, you might want to consider adding the instrumentation packages as well for automatic instrumentation of common libraries.
opentelemetry-api = ">=1.30.0" opentelemetry-sdk = ">=1.30.0" +opentelemetry-instrumentation-fastapi = ">=0.44b0" +opentelemetry-instrumentation-httpx = ">=0.44b0"ibis-server/app/mdl/substitute.py (3)
18-18: Correctly implemented span for substitute methodThe span decorator is properly configured with the appropriate name and span kind. This will provide valuable tracing information about the method's execution.
Consider adding attributes to the span to provide more context about the operation.
-@tracer.start_as_current_span("substitute", kind=trace.SpanKind.INTERNAL) +@tracer.start_as_current_span("substitute", kind=trace.SpanKind.INTERNAL) def substitute(self, sql: str, write: str | None = None) -> str: + current_span = trace.get_current_span() + current_span.set_attribute("sql.query", sql) + current_span.set_attribute("sql.dialect", self.data_source.value) + if write: + current_span.set_attribute("sql.write_dialect", write)
18-36: Consider adding internal spans for better granularityFor a complex method like this, consider adding nested spans for key operations to provide more detailed tracing.
@tracer.start_as_current_span("substitute", kind=trace.SpanKind.INTERNAL) def substitute(self, sql: str, write: str | None = None) -> str: - ast = parse_one(sql, dialect=self.data_source.value) - root = build_scope(ast) + with tracer.start_as_current_span("parse_sql"): + ast = parse_one(sql, dialect=self.data_source.value) + + with tracer.start_as_current_span("build_scope"): + root = build_scope(ast) + + with tracer.start_as_current_span("replace_models"): for scope in root.traverse(): for alias, (node, source) in scope.selected_sources.items(): if isinstance(source, exp.Table): model = self._find_model(source) if model is None: raise SubstituteError(f"Model not found: {source}") source.replace( exp.Table( catalog=quote(self.manifest["catalog"]), db=quote(self.manifest["schema"]), this=quote(model["name"]), alias=quote(alias), ) ) - return ast.sql(dialect=write) + + with tracer.start_as_current_span("generate_sql"): + result = ast.sql(dialect=write) + return result
46-50: Consider adding tracing to _find_model methodThis helper method might benefit from tracing, especially if model lookups could be a performance bottleneck.
+@tracer.start_as_current_span("_find_model", kind=trace.SpanKind.INTERNAL) def _find_model(self, source: exp.Table) -> dict | None: catalog = source.catalog or "" schema = source.db or "" table = source.name - return self.model_dict.get(f"{catalog}.{schema}.{table}", None) + key = f"{catalog}.{schema}.{table}" + current_span = trace.get_current_span() + current_span.set_attribute("model.key", key) + result = self.model_dict.get(key, None) + current_span.set_attribute("model.found", result is not None) + return resultibis-server/app/util.py (1)
104-105: Consider removing unnecessary functionThe
get_headerfunction simply returns its input without any transformation, making it potentially redundant if not used for specific purposes elsewhere.-def get_header(headers: Headers) -> Headers: - return headersUnless this function is used elsewhere as a dependency injection point or for future extensibility, it could be removed to simplify the codebase.
ibis-server/app/routers/v3/connector.py (1)
78-93: Consider adding error attributes to spansWhile the span implementation is correct, consider adding error details as span attributes when exceptions occur for better observability.
with tracer.start_as_current_span( name=span_name, kind=trace.SpanKind.SERVER, context=build_context(headers) ) as span: try: validator = Validator( Connector(data_source, dto.connection_info), Rewriter(dto.manifest_str, data_source=data_source, experiment=True), ) await validator.validate(rule_name, dto.parameters, dto.manifest_str) return Response(status_code=204) + except Exception as e: + span.set_status(trace.StatusCode.ERROR) + span.record_exception(e) + raiseibis-server/app/model/connector.py (2)
71-77: Consider adding span attributes for query detailsWhile the span implementation is correct, adding attributes like query length or complexity metrics would enhance observability.
@tracer.start_as_current_span("connector_query", kind=trace.SpanKind.CLIENT) def query(self, sql: str, limit: int) -> pd.DataFrame: + with trace.use_span(trace.get_current_span()) as span: + span.set_attribute("db.statement.length", len(sql)) + span.set_attribute("db.result.limit", limit) return self.connection.sql(sql).limit(limit).to_pandas()
111-122: Distinguish spans between connector implementationsConsider using more specific span names to differentiate between different connector implementations.
-@tracer.start_as_current_span("connector_query", kind=trace.SpanKind.CLIENT) +@tracer.start_as_current_span("canner_connector_query", kind=trace.SpanKind.CLIENT) def query(self, sql: str, limit: int) -> pd.DataFrame: # ...ibis-server/app/routers/v2/connector.py (2)
41-46: Consider adding span attributes for enhanced observabilityWhile the current implementation correctly creates spans, adding attributes to these spans would provide richer context for debugging and monitoring.
Consider enhancing the spans with relevant attributes. For example:
with tracer.start_as_current_span( name=span_name, kind=trace.SpanKind.SERVER, context=build_context(headers) -): +) as span: + span.set_attribute("data_source", str(data_source)) + span.set_attribute("operation", "query") + # Add other relevant attributes specific to the endpointThis pattern could be applied to all endpoints, customizing the attributes based on the specific operation and available context data.
Also applies to: 67-70, 89-92, 104-107, 124-126, 139-142, 157-160
41-56: Add error handling to spansThe current implementation doesn't explicitly handle exceptions within spans. Adding proper error handling would ensure errors are properly recorded in traces.
Consider updating the span implementations to include error handling:
with tracer.start_as_current_span( name=span_name, kind=trace.SpanKind.SERVER, context=build_context(headers) -): +) as span: + try: # Existing implementation + except Exception as e: + span.record_exception(e) + span.set_status(trace.StatusCode.ERROR, str(e)) + raiseThis pattern ensures that exceptions are captured in the trace, providing better observability for debugging issues.
Also applies to: 67-80, 89-95, 104-110, 124-129, 139-147, 157-171
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ibis-server/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
ibis-server/README.md(1 hunks)ibis-server/app/mdl/rewriter.py(6 hunks)ibis-server/app/mdl/substitute.py(1 hunks)ibis-server/app/model/connector.py(8 hunks)ibis-server/app/routers/v2/connector.py(5 hunks)ibis-server/app/routers/v3/connector.py(3 hunks)ibis-server/app/util.py(2 hunks)ibis-server/justfile(1 hunks)ibis-server/pyproject.toml(1 hunks)
🧰 Additional context used
🪛 LanguageTool
ibis-server/README.md
[grammar] ~82-~82: The pronoun ‘We’ must be used with a non-third-person form of a verb.
Context: ...ash just run ``` ### Enable Tracing We uses OpenTelemetry as its tracing framework....
(NON3PRS_VERB)
🪛 markdownlint-cli2 (0.17.2)
ibis-server/README.md
84-84: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci
🔇 Additional comments (39)
ibis-server/justfile (1)
26-27: Well-implemented tracing configurationThe
run-trace-consolecommand correctly configures OpenTelemetry instrumentation with appropriate settings. Exporting traces to console while disabling metrics is a good approach for local development and debugging.ibis-server/app/mdl/substitute.py (2)
1-1: Good addition of OpenTelemetry importThe OpenTelemetry trace module import is correctly added.
9-10: Well-structured tracer initializationCreating the tracer with
__name__is the recommended approach as it associates spans with the correct module.ibis-server/app/util.py (5)
7-10: Good import organization for OpenTelemetryThe necessary imports for OpenTelemetry and header handling are properly organized. This is essential for setting up distributed tracing correctly.
14-14: Appropriate tracer instantiationThe tracer is correctly initialized using the module name, which is a best practice for OpenTelemetry instrumentation.
17-19: Well-structured span decoration for base64_to_dictThe tracing span is properly configured with a descriptive name and the correct kind for internal operations.
22-22: Well-structured span decoration for to_jsonSimilar to the previous function, this is properly instrumented with a descriptive span name and appropriate kind.
98-101: Properly implemented context extractionThe
build_contextfunction correctly handles header propagation for distributed tracing. The null check is important for robustness.ibis-server/app/mdl/rewriter.py (7)
7-8: Clean import for OpenTelemetryThe import for the trace module is properly isolated and clearly indicates the dependency.
25-26: Correct tracer initializationThe tracer is initialized with the module name, following OpenTelemetry best practices.
45-49: Appropriate span for _transpile methodThe span is correctly configured with a descriptive name and the appropriate SpanKind.INTERNAL for an internal operation.
51-61: Well-structured tracing for rewrite methodThe span properly wraps the entire method, which is a good approach for capturing the entire operation duration.
63-71: Comprehensive tracing for manifest extractionThe span correctly wraps the manifest extraction logic, including potential exception handling, which is valuable for debugging.
95-105: Use of CLIENT span kind for external rewriteExcellent choice of using SpanKind.CLIENT for the external rewriter's method, which correctly indicates that this operation involves communication with an external service.
115-122: Consistent tracing for embedded rewriteThe span for the embedded rewrite method maintains consistency with the external rewrite method, while correctly using INTERNAL kind.
ibis-server/app/routers/v3/connector.py (8)
3-6: Properly structured imports for tracing and headersThe imports are cleanly organized with minimal changes to include OpenTelemetry tracing and FastAPI header support.
21-24: Updated util import and tracer initializationThe import for the new
build_contextfunction and the tracer initialization are properly implemented.
33-33: Appropriate header parameter additionThe header parameter is correctly added as an optional annotated parameter, following FastAPI best practices.
35-48: Well-structured span for query endpointThe span is properly configured with a dynamic name based on the operation and data source, and correctly uses the SERVER kind. The context extraction from headers is also properly implemented.
52-59: Consistent tracing approach for dry_plan endpointThe span follows the same pattern as the query endpoint, maintaining consistency across endpoints.
63-74: Dynamic span naming for data source specific endpointGood practice of including the data source in the span name for better context in traces.
97-108: Synchronous function with tracingGood implementation of tracing for a synchronous function, maintaining the same pattern as the async endpoints.
112-129: Comprehensive tracing for complex model_substitute endpointThe span properly wraps the entire operation, which involves multiple steps (substitution, dry run, etc.).
ibis-server/app/model/connector.py (6)
18-19: Clean import for OpenTelemetryThe import is isolated and clearly indicates the dependency.
34-35: Correct tracer initializationThe tracer uses the module name for proper identification.
38-55: Tracing for connector initializationGood practice to trace the initialization process, which helps identify startup performance issues.
94-97: Well-named span for error handling methodThe span name clearly indicates its purpose for error message generation.
158-179: Detailed nested span for BigQuery schema handlingExcellent detail in tracing specific error handling logic with a dedicated span. This will help diagnose schema-related issues.
196-213: Appropriate spans for DuckDB operationsThe spans are correctly configured with INTERNAL kind, reflecting that these operations don't cross service boundaries.
ibis-server/app/routers/v2/connector.py (10)
3-5: Added OpenTelemetry tracing imports correctlyThe imports are properly organized and include the necessary OpenTelemetry trace module.
22-22: Successfully imported build_context utilityThe
build_contextutility function is appropriately imported for handling tracing context.
25-25: Proper tracer initializationThe tracer is correctly initialized using the module name as per OpenTelemetry best practices.
33-56: Well-implemented tracing for query endpointThe implementation effectively wraps the existing functionality with a tracing span. The span name is dynamically generated based on the
dry_runparameter, which provides valuable context.
60-80: Proper tracing implementation for validate endpointThe tracing span correctly wraps the validation logic with an appropriate span name.
83-95: Appropriate tracing for metadata table endpointThe implementation follows the established pattern for tracing consistently.
98-110: Good tracing implementation for constraints endpointThe span naming convention is consistent with other endpoints, which will help with trace analysis.
119-129: Appropriate tracing for dry_plan endpointThe span name is clear and the tracing implementation is correct.
133-147: Good implementation for dry_plan_for_data_sourceThe tracing implementation is consistent with other endpoints.
151-171: Well-structured tracing for model_substitute endpointThe tracing implementation correctly encompasses all operations within the endpoint.
There was a problem hiding this comment.
Thanks @douenergy. It looks great!
Description
This PR invokes OpenTelemetry as the tracing framework. It makes us easily export the tracing log to an external framework (e.g., datadog, jaeger, GCP cloud tracing, ..).
How to test
Try to use the following
justcommand to start the ibis server, which will export the tracing log the console:Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation