fix(tracing): fix tracer provider initialization logic #381
Conversation
|
|
1 similar comment
|
|
| enable_report = _str_to_bool(os.getenv("TRACE_ENABLE_REPORT", "false")) | ||
| enable_debug = _str_to_bool(os.getenv("TRACE_ENABLE_DEBUG", "false")) | ||
|
|
||
| if enable_report and _has_existing_trace_provider(): |
There was a problem hiding this comment.
What is the use case for this?
If the user has already configured a trace_provider at the beginning, do they still need to additionally set TRACE_ENABLE_REPORT in order to reuse the existing trace_provider?
If so, that seems rather odd. The user has already configured the trace_provider according to the OpenTelemetry specification (In our cases), but now they also need to learn or inspect the code to discover that there is an additional TRACE_ENABLE_REPORT environment variable.
There was a problem hiding this comment.
My main point is this: if the user has already configured a global trace_provider in their code (and has likely also set up the corresponding exporter), shouldn't the system follow that configuration by default? It seems unnecessar to require the user to additionally set the TRACE_ENABLE_REPORT environment variable in order to activate logic that leverages an already-configured trace_provider.
There was a problem hiding this comment.
Pull request overview
This PR fixes the tracer provider initialization logic in the OpenTelemetry tracing wrapper. The primary goal is to optimize when a TracerProvider is created based on environment variable settings.
Changes:
- Modified
_get_ot_tracer_inner()to read tracing environment variables upfront and use them to control provider initialization - Changed the logic to only create a full TracerProvider when tracing is actually enabled (via TRACE_ENABLE_REPORT or TRACE_ENABLE_DEBUG)
- Added a NoOpTracerProvider when neither tracing option is enabled to avoid unnecessary overhead
Comments suppressed due to low confidence (1)
src/agentscope_runtime/engine/tracing/wrapper.py:975
- This tracer provider initialization logic lacks test coverage. Given that the repository has comprehensive unit tests for other components, consider adding tests to verify the behavior when TRACE_ENABLE_REPORT and TRACE_ENABLE_DEBUG are set to different combinations (true/false, true/true, false/true, false/false), and when an existing trace provider is or isn't present.
enable_report = _str_to_bool(os.getenv("TRACE_ENABLE_REPORT", "false"))
enable_debug = _str_to_bool(os.getenv("TRACE_ENABLE_DEBUG", "false"))
if enable_report and _has_existing_trace_provider():
return ot_trace.get_tracer("agentscope_runtime")
# Create TracerProvider if either report or debug is enabled
if enable_report or enable_debug:
resource = Resource(
attributes={
SERVICE_NAME: _get_service_name(),
SERVICE_VERSION: os.getenv("SERVICE_VERSION", "1.0.0"),
"source": "agentscope_runtime-source",
},
)
provider = TracerProvider(resource=resource)
if enable_report:
span_exporter = BatchSpanProcessor(
OTLPSpanGrpcExporter(
endpoint=os.getenv("TRACE_ENDPOINT", ""),
headers=f"Authentication="
f"{os.getenv('TRACE_AUTHENTICATION', '')}",
),
)
provider.add_span_processor(span_exporter)
if enable_debug:
span_logger = BatchSpanProcessor(ConsoleSpanExporter())
provider.add_span_processor(span_logger)
else:
provider = ot_trace.NoOpTracerProvider()
tracer = ot_trace.get_tracer(
"agentscope_runtime",
tracer_provider=provider,
)
return tracer
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| enable_report = _str_to_bool(os.getenv("TRACE_ENABLE_REPORT", "false")) | ||
| enable_debug = _str_to_bool(os.getenv("TRACE_ENABLE_DEBUG", "false")) | ||
|
|
||
| if enable_report and _has_existing_trace_provider(): |
There was a problem hiding this comment.
The condition checking enable_report and _has_existing_trace_provider() introduces a behavioral change from the previous logic. Previously, any existing trace provider would be reused regardless of the environment variable settings. Now, an existing provider is only reused if enable_report is true. This means if enable_debug=true but enable_report=false, a new TracerProvider will be created even if one already exists. Document this behavior change or consider whether the condition should be (enable_report or enable_debug) and _has_existing_trace_provider() to preserve the original behavior of reusing any existing provider.
| if enable_report and _has_existing_trace_provider(): | |
| # Reuse an existing tracer provider whenever tracing is enabled | |
| # (report or debug), preserving the original behavior of reusing | |
| # any previously configured provider. | |
| if (enable_report or enable_debug) and _has_existing_trace_provider(): |
| if enable_debug: | ||
| span_logger = BatchSpanProcessor(ConsoleSpanExporter()) | ||
| provider.add_span_processor(span_logger) | ||
| else: |
There was a problem hiding this comment.
The comment states "Create TracerProvider if either report or debug is enabled" which accurately describes the condition on line 945. However, consider adding a comment for the else block at line 968-969 to clarify that a NoOpTracerProvider is used when tracing is completely disabled, as this is an important aspect of the initialization logic.
| else: | |
| else: | |
| # Use a NoOpTracerProvider when both reporting and debug tracing are disabled |
2 participants
Description
[Describe what this PR does and why]
Related Issue: Fixes #[issue_number] or Relates to #[issue_number]
Security Considerations: [If applicable, especially for sandbox changes]
Type of Change
Component(s) Affected
Checklist
Testing
[How to test these changes]
Additional Notes
[Optional: any other context]