fix(instrumentation): Fixes gRPC exporter initialisation with insecure OTLP #3481
Conversation
|
|
WalkthroughThe PR changes init_spans_exporter to parse API endpoints and select HTTP or gRPC OTLP exporters based on URL scheme (http/https/grpc/grpcs or scheme-less), normalizing endpoints and insecure flags. It also adds tests validating scheme handling and endpoint normalization. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant init_spans_exporter
participant urlparse
participant HTTPExporter
participant GRPCExporter
Client->>init_spans_exporter: init_spans_exporter(api_endpoint, headers)
init_spans_exporter->>urlparse: parse(api_endpoint)
urlparse-->>init_spans_exporter: parsed(scheme, netloc, path)
alt scheme is http or https
init_spans_exporter->>HTTPExporter: instantiate(endpoint=normalized+"/v1/traces", headers)
HTTPExporter-->>init_spans_exporter: return OTLPSpanExporter
else scheme is grpc
init_spans_exporter->>GRPCExporter: instantiate(endpoint=netloc, headers, insecure=true)
GRPCExporter-->>init_spans_exporter: return OTLPSpanExporter
else scheme is grpcs
init_spans_exporter->>GRPCExporter: instantiate(endpoint=netloc, headers, insecure=false)
GRPCExporter-->>init_spans_exporter: return OTLPSpanExporter
else no scheme
init_spans_exporter->>GRPCExporter: instantiate(endpoint=trimmed_input, headers, insecure=true)
GRPCExporter-->>init_spans_exporter: return OTLPSpanExporter
end
init_spans_exporter-->>Client: SpanExporter instance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @nirga, could you please take a look at this PR when you have a moment? Thanks! |
|
Thanks @boscoraju-snyk & @LeHyperion - can you change the PR to ready for review and sign the CLA? |
Sure! |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 386ebc4 in 2 minutes and 18 seconds. Click for details.
- Reviewed
106lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/traceloop-sdk/tests/test_sdk_initialization.py:15
- Draft comment:
The parameterized tests for init_spans_exporter are clear and comprehensive, covering both HTTP and gRPC URL formats (with case-insensitive schemes and whitespace). - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py:315
- Draft comment:
When constructing the HTTP exporter endpoint (appending '/v1/traces'), consider handling potential trailing slashes to avoid double slashes in the URL. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 40% vs. threshold = 50% The comment is about code that was changed in the diff (the HTTP exporter endpoint construction). The suggestion to use.rstrip('/')is reasonable and would prevent double slashes. However, I need to consider: 1) Is this actually a problem? Double slashes in URL paths are typically normalized by HTTP clients and shouldn't cause issues. 2) Is this an edge case that's unlikely to occur? 3) The comment is making a suggestion for code quality improvement, which according to the rules is good if actionable and clear. The suggestion is clear and actionable. But is it necessary? Most HTTP libraries handle double slashes gracefully. This feels like a minor code quality suggestion rather than a critical bug. While double slashes might look inelegant, most HTTP clients and servers handle them correctly by normalizing the path. This might be an overly cautious suggestion that doesn't address a real problem. The comment also doesn't provide evidence that this is actually causing issues or that users commonly pass endpoints with trailing slashes. Even if HTTP clients handle double slashes, it's still better practice to construct clean URLs. The fix is trivial (adding.rstrip('/')) and prevents potential edge cases. The comment is actionable, clear, and improves code quality without being overly pedantic. It's a reasonable defensive programming practice. This is a reasonable code quality suggestion that's actionable and clear. While double slashes may not cause actual failures, preventing them is a simple improvement. However, without evidence that this is a real problem users face, it's borderline whether this meets the threshold of "clearly a code change required."
3. packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py:313
- Draft comment:
The match/case logic clearly differentiates URL schemes and correctly defaults empty/unsupported schemes to insecure gRPC. Ensure this behavior remains documented and tested. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_vt3HGnwuy9K8zxsH
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (1)
294-330: Consider adding URL validation and error handling.The function doesn't validate whether the parsed URL is well-formed or handle potential parsing errors. While
urlparseis generally permissive, it's good practice to validate critical inputs, especially for security-sensitive configuration like endpoint URLs.Consider adding validation:
def init_spans_exporter(api_endpoint: str, headers: Dict[str, str]) -> SpanExporter: """ Initialize a span exporter based on the endpoint URL scheme. Supported schemes: - http:// or https:// → HTTP exporter - grpc:// → gRPC exporter (insecure) - grpcs:// → gRPC exporter (secure/TLS) - No scheme → gRPC exporter (insecure, for backward compatibility) Args: api_endpoint: The endpoint URL (with or without scheme) headers: Headers to include with the exporter requests Returns: SpanExporter: Configured HTTP or gRPC exporter + + Raises: + ValueError: If the endpoint URL is invalid """ + if not api_endpoint or not api_endpoint.strip(): + raise ValueError("api_endpoint cannot be empty") + parsed = urlparse(api_endpoint.strip()) + + # Validate netloc is present for schemes that require it + if parsed.scheme in ("http", "https", "grpc", "grpcs") and not parsed.netloc: + raise ValueError(f"Invalid endpoint URL: {api_endpoint}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/traceloop-sdk/tests/test_sdk_initialization.py(2 hunks)packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/traceloop-sdk/tests/test_sdk_initialization.pypackages/traceloop-sdk/traceloop/sdk/tracing/tracing.py
🧬 Code graph analysis (1)
packages/traceloop-sdk/tests/test_sdk_initialization.py (1)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (1)
init_spans_exporter(294-330)
🔇 Additional comments (4)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (2)
4-4: LGTM!The import of
urlparseis appropriate for parsing endpoint URLs and determining the scheme.
313-330: No action needed—Python version requirement is already satisfied.The project requires Python >= 3.10, which fully supports the
match/casesyntax. This code change is compatible with the project's minimum supported Python version and does not introduce any compatibility issues.Likely an incorrect or invalid review comment.
packages/traceloop-sdk/tests/test_sdk_initialization.py (2)
2-2: LGTM!The
patchimport is appropriately used for mocking the gRPC exporter initialization in the tests.
25-39: LGTM!The test provides comprehensive coverage of gRPC endpoint handling:
- Tests both
grpc://andgrpcs://schemes- Verifies case-insensitive scheme handling
- Tests whitespace trimming
- Verifies the default behavior for endpoints without a scheme (backward compatibility)
- Correctly validates that
netlocis extracted and theinsecureflag is set appropriately
…p#3480) fix(sdk): ensures correct HTTP endpoint construction
e5aac97 to
6fdcb4b
Compare
|
@nirga Could you please take another look at this when you have a moment? |
|
@boscoraju-snyk - can you fix the lint issues? |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/traceloop-sdk/tests/test_sdk_initialization.py (1)
2-38: HTTP exporter tests look good; wrap patch/assert lines to satisfy Flake8The HTTP tests nicely cover scheme handling, endpoint normalization (
/v1/traces, trailing slash, whitespace) and type selection, and they correctly patchOTLPSpanExporter.__init__to assert the constructed endpoint and headers. To avoid Flake8 line-length violations in thewith patch.objectandassert_called_once_withlines, consider breaking them like this:- with patch.object(OTLPSpanExporter, "__init__", return_value=None) as mock: - init_spans_exporter(endpoint, {}) - mock.assert_called_once_with(endpoint=expected_endpoint, headers={}) + with patch.object( + OTLPSpanExporter, + "__init__", + return_value=None, + ) as mock: + init_spans_exporter(endpoint, {}) + mock.assert_called_once_with( + endpoint=expected_endpoint, + headers={}, + )This keeps behavior the same while making the tests lint‑friendly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/traceloop-sdk/tests/test_sdk_initialization.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/traceloop-sdk/tests/test_sdk_initialization.py
🧬 Code graph analysis (1)
packages/traceloop-sdk/tests/test_sdk_initialization.py (1)
packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (1)
init_spans_exporter(294-333)
@nirga Sure, I have resolved the lint issues now. |
feat(instrumentation): ...orfix(instrumentation): ....This PR fixes an issue where the gRPC exporter was not being initialised correctly when using insecure OTLP collectors. It now correctly handles different URL schemes (grpc://, grpcs://, and no scheme) for the OTLP collector endpoint, ensuring that the
insecureflag is properly set during gRPC exporter initialisation and that the endpoint is parsed correctly.Important
Fixes gRPC exporter initialization in
init_spans_exporter()to correctly handle URL schemes and set theinsecureflag, with tests added for verification.init_spans_exporter()intracing.pyto handlegrpc://,grpcs://, and no scheme correctly.insecureflag based on URL scheme:grpc://and no scheme are insecure,grpcs://is secure.TestInitSpansExporterintest_sdk_initialization.pyto testinit_spans_exporter()with various URL schemes.unittest.mock.patchto verifyOTLPSpanExporterinitialization parameters.urlparseimport intracing.pyfor URL parsing.This description was created by
for 386ebc4. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.