feat: otel support for grpc plugins #2021
Conversation
WalkthroughThis change introduces comprehensive OpenTelemetry tracing support across the router plugin architecture. It adds new configuration types for tracing and IP anonymization, implements tracing initialization and interceptors, and integrates tracing into the HTTP client and gRPC server setup. Extensive tests are provided to validate tracing, span attributes, IP anonymization, and header propagation. New dependencies are added to support these features. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
SkArchon
force-pushed
the
milinda/eng-7173-otel-support-for-grpc-plugins
branch
from
3c60990 to
1507e5a
Compare
SkArchon
requested review from
a team,
Noroth,
StarpTech and
devsergiy
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
demo/pkg/subgraphs/projects/go.mod (1)
3-3: Invalid Go version specified.Go version 1.24.1 does not exist. The latest stable version is 1.23.x. Please update to a valid Go version.
-go 1.24.1 +go 1.23.5
🧹 Nitpick comments (1)
router-plugin/plugin.go (1)
50-74: Option functions are well-implemented.The functional options pattern is correctly implemented. However, the comment on line 57 could be more accurate.
Consider updating the comment to be more precise:
-// WithTracing enables tracing for the plugin. -// This includes creating a tracing interceptor +// WithTracing enables tracing for the plugin. +// The tracing interceptor will be created during plugin initialization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
demo/pkg/subgraphs/projects/go.sumis excluded by!**/*.sumrouter-plugin/go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
demo/pkg/subgraphs/projects/go.mod(1 hunks)demo/pkg/subgraphs/projects/src/main.go(1 hunks)router-plugin/go.mod(1 hunks)router-plugin/httpclient/client.go(7 hunks)router-plugin/plugin.go(2 hunks)router-plugin/tracing/interceptor.go(1 hunks)router-plugin/tracing/tracer.go(1 hunks)router/core/graph_server.go(2 hunks)router/pkg/routerplugin/grpc_plugin.go(4 hunks)router/pkg/routerplugin/grpc_plugin_client.go(2 hunks)router/pkg/routerplugin/metadata_carrier.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
router/pkg/routerplugin/grpc_plugin_client.go (1)
router/pkg/trace/tracer.go (1)
Inject(42-46)
router-plugin/tracing/interceptor.go (1)
router-plugin/tracing/tracer.go (1)
TracingOptions(20-23)
router/core/graph_server.go (2)
router/pkg/trace/config.go (1)
ExporterConfig(28-43)router/pkg/routerplugin/grpc_plugin.go (2)
NewGRPCPlugin(42-66)GRPCPluginConfig(17-22)
🔇 Additional comments (11)
router/core/graph_server.go (1)
1349-1363: LGTM! Well-implemented tracing configuration propagation.The approach of using environment variables to pass tracing configuration to plugins is sound and aligns with the past review comments. The filtering logic correctly identifies enabled exporters and the JSON serialization provides a clean way to pass structured configuration.
router/pkg/routerplugin/grpc_plugin_client.go (1)
133-135: LGTM! Proper OpenTelemetry context propagation implementation.The implementation correctly:
- Creates fresh metadata for each call to avoid cross-contamination
- Uses the standard OpenTelemetry inject mechanism via
metadataCarrier- Properly attaches metadata to the outgoing context before invoking the gRPC call
This follows OpenTelemetry best practices for distributed tracing across gRPC boundaries.
demo/pkg/subgraphs/projects/src/main.go (1)
18-33: LGTM! Clean enhancement for observability.The addition of service metadata and tracing configuration enhances observability without adding complexity. The use of constants for service name and version is clean, and the options pattern is applied correctly to enable tracing and provide service identification.
router/pkg/routerplugin/metadata_carrier.go (1)
8-30: LGTM! Well-implemented metadata carrier for OpenTelemetry propagation.The
metadataCarrierimplementation correctly provides a case-insensitive wrapper around gRPC metadata for OpenTelemetry context propagation. Key strengths:
- Consistent case normalization using
strings.ToLowerfor all operations- Proper handling of missing values in
Getmethod- Clean embedding of
metadata.MD- Standard implementation pattern for text map propagators
This enables proper trace context propagation across gRPC boundaries.
demo/pkg/subgraphs/projects/go.mod (1)
8-27: OpenTelemetry dependencies look good.The dependency updates and additions properly support the OpenTelemetry tracing implementation across the codebase.
router-plugin/httpclient/client.go (1)
54-56: Well-implemented OpenTelemetry tracing for HTTP client.The tracing implementation is properly structured with:
- No-op tracer initialization to avoid nil checks
- Proper span lifecycle management with defer
- Correct error recording and status setting
- Consistent context propagation using the global propagator
Also applies to: 104-109, 152-180, 189-189, 271-271
router/pkg/routerplugin/grpc_plugin.go (1)
21-21: Correct implementation of environment variable parameter passing.The implementation properly handles environment variable construction by:
- First appending the current environment
- Then adding parameters, allowing them to override existing values
- Setting
SkipHostEnv: trueto have full control over the environmentThis ensures consistent and predictable parameter passing to plugin processes.
Also applies to: 39-39, 64-64, 99-104, 110-110
router-plugin/tracing/tracer.go (1)
48-139: Well-structured OpenTelemetry tracer initialization.The implementation follows best practices:
- Proper resource creation with service attributes and system metadata
- Reasonable span limits and batch processing configuration
- Support for multiple exporters with sensible defaults
- Correct global tracer provider and propagator setup
- Good error handling throughout
router-plugin/plugin.go (3)
3-10: Import additions are appropriate.The new imports for
fmtand the tracing package are necessary for error formatting and OpenTelemetry support.
35-48: Well-structured metadata encapsulation.The addition of
serveConfigandmetadatafields toRouterPlugin, along with the newRouterPluginMetadatastruct, provides a clean separation of concerns. The change toPluginOptiontype correctly reflects that options now configure theRouterPlugininstance.
107-118: Clean implementation of server setup and serve method.The
GRPCServerfunction correctly merges server options, preserving both default and custom options. TheServemethod properly delegates to the plugin framework.
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
This reverts commit 1507e5a.
…-support-for-grpc-plugins
Router image scan passed✅ No security vulnerabilities found in image: |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
router-plugin/tracing/tracer.go (1)
16-16: Standardize OpenTelemetry semconv dependency version.This file imports
semconv/v1.20.0while other parts of the codebase usev1.26.0. Consider standardizing on a single version for consistency to avoid import conflicts and ensure uniform tracing behavior.router-plugin/plugin.go (1)
138-148: Fix error handling order.The error check for
CreateTracingInterceptorshould occur before appending the interceptor togrpcOptsto avoid appending a nil interceptor if the function fails.
🧹 Nitpick comments (1)
router-plugin/tracing/propogator.go (1)
1-1: Fix filename typo: "propogator" should be "propagator".The filename contains a typo. Please rename the file from
propogator.gotopropagator.goto match the correct spelling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
router-plugin/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
demo/pkg/subgraphs/projects/src/main.go(1 hunks)router-plugin/config/settings.go(1 hunks)router-plugin/go.mod(1 hunks)router-plugin/httpclient/client.go(7 hunks)router-plugin/plugin.go(5 hunks)router-plugin/tracing/interceptor.go(1 hunks)router-plugin/tracing/propogator.go(1 hunks)router-plugin/tracing/redact.go(1 hunks)router-plugin/tracing/tracer.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- demo/pkg/subgraphs/projects/src/main.go
- router-plugin/config/settings.go
🚧 Files skipped from review as they are similar to previous changes (3)
- router-plugin/go.mod
- router-plugin/httpclient/client.go
- router-plugin/tracing/interceptor.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: in the cosmo router project, when extending json schema validation for security-sensitive fields lik...
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
Applied to files:
router-plugin/tracing/tracer.go
📚 Learning: in the cosmo router project, required field validation for jwks configuration (secret, algorithm, ke...
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Applied to files:
router-plugin/tracing/tracer.gorouter-plugin/plugin.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
router-plugin/tracing/propogator.go (1)
12-31: LGTM! Propagator mapping implementation is correct.The function correctly maps configuration propagator types to their OpenTelemetry implementations. The B3 propagator configuration with both multiple and single header injection is appropriate for maximum compatibility.
router-plugin/tracing/tracer.go (2)
84-96: Excellent security implementation for IP anonymization.The IP anonymization implementation properly supports both hashing (using SHA256) and redaction methods. The hash function provides a secure one-way transformation while the redact function completely obscures sensitive data.
148-204: LGTM! Exporter configuration handles security correctly.The exporter creation properly handles both OTLP HTTP and gRPC variants with appropriate TLS configuration based on the URL scheme. The endpoint parsing and security options are implemented correctly.
router-plugin/tracing/redact.go (3)
7-7: Another semconv version inconsistency.This file uses
semconv/v1.17.0whiletracer.gousesv1.20.0. This creates another version inconsistency that should be standardized across the tracing package.
65-74: Efficient attribute redaction implementation.The in-place modification of attribute values in the OnEnd method is a clever and memory-efficient approach. The use of a map for key lookups ensures O(1) performance for attribute matching.
17-21: Appropriate selection of sensitive attributes.The choice to redact
HTTPClientIPKeyandNetSockPeerAddrKeycorrectly targets the most common sources of IP address leakage in HTTP tracing.router-plugin/plugin.go (3)
56-61: Well-structured configuration management.The
RouterPluginConfigstruct provides a clean separation of concerns with appropriate fields for service metadata and tracing options. The optional error handler provides good flexibility for different use cases.
118-125: Proper environment-based configuration loading.The startup configuration loading from the environment variable is appropriate for the plugin architecture, allowing dynamic configuration without requiring plugin recompilation.
129-137: Good fallback logic for service metadata.The service name and version fallback logic ensures sensible defaults while allowing customization through plugin options.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
router-plugin/tracing/tracer.go (2)
21-28: Clarify the need for exported fields in TracingOptionsA previous reviewer asked about the reason for these fields being exported. Please provide clarification on whether these fields need to be publicly accessible or if they could be unexported.
16-16: Standardize semconv version to v1.26.0As noted in a previous review, this file uses
semconv/v1.20.0while other parts of the codebase use different versions. Please update to v1.26.0 for consistency.
🧹 Nitpick comments (1)
router-tests/plugintest/hello/src/schema.graphql (1)
8-8: Add newline at end of filetype Query { run(input: String!): Result! } +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
router-tests/plugintest/hello/generated/mapping.jsonis excluded by!**/generated/**router-tests/plugintest/hello/generated/service.pb.gois excluded by!**/*.pb.go,!**/generated/**router-tests/plugintest/hello/generated/service.protois excluded by!**/generated/**router-tests/plugintest/hello/generated/service.proto.lock.jsonis excluded by!**/generated/**router-tests/plugintest/hello/generated/service_grpc.pb.gois excluded by!**/*.pb.go,!**/generated/**
📒 Files selected for processing (8)
router-plugin/config/settings.go(1 hunks)router-plugin/httpclient/client.go(7 hunks)router-plugin/tracing/interceptor.go(1 hunks)router-plugin/tracing/redact.go(1 hunks)router-plugin/tracing/tracer.go(1 hunks)router-tests/plugintest/hello/src/schema.graphql(1 hunks)router-tests/plugintest/setup.go(1 hunks)router-tests/plugintest/tracing_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- router-plugin/tracing/interceptor.go
- router-plugin/tracing/redact.go
- router-plugin/httpclient/client.go
- router-tests/plugintest/setup.go
- router-plugin/config/settings.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: in the cosmo router project, when extending json schema validation for security-sensitive fields lik...
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
Applied to files:
router-plugin/tracing/tracer.go
📚 Learning: in the cosmo router project, required field validation for jwks configuration (secret, algorithm, ke...
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Applied to files:
router-plugin/tracing/tracer.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
router-tests/plugintest/tracing_test.go (1)
24-614: Excellent test coverage!The test suite is comprehensive and well-structured, covering:
- Various tracing disabled scenarios
- Span verification with default and custom values
- IP anonymization methods (disabled, redact, hash)
- HTTP client tracing behavior
The use of table-driven tests and descriptive test names makes the test suite maintainable and easy to understand.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
router-tests/plugintest/setup.go (1)
58-58: Consider adding flexibility for additional gRPC server options.The server is created with an empty options slice, which limits test flexibility if additional server options are needed.
Consider adding server options to the test config:
type PluginTestConfig[T any] struct { StartupConfig config.StartupConfig RouterPluginConfig config.RouterPluginConfig + ServerOptions []grpc.ServerOption RegisterServiceFunc func(grpc.ServiceRegistrar) CreateClientFunc func(conn *grpc.ClientConn) T }Then use it in the setup:
- grpcServer := server([]grpc.ServerOption{}) + grpcServer := server(testConfig.ServerOptions)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
router-tests/plugintest/setup.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build-router
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
router-tests/plugintest/setup.go (4)
1-16: LGTM! Clean package structure and imports.The package declaration and imports are well-organized and appropriate for a test utility. All imports appear to be used and follow Go conventions.
18-25: LGTM! Good test service pattern.The HelloService implementation follows best practices by embedding the unimplemented server for forward compatibility and using a function field to allow flexible test behavior injection.
33-35: LGTM! Appropriate buffer size for testing.The 1MB buffer size is reasonable for test scenarios and follows good naming conventions.
37-42: LGTM! Well-designed configuration struct.The PluginTestConfig struct provides good flexibility for test scenarios through its function fields and configuration options. The generic approach maintains type safety.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
router-tests/plugintest/tracing_test.go (1)
12-12: Use consistent semconv versionBased on the past review comment, this import should be updated to use a consistent semconv version. The codebase appears to be using semconv v1.20.0 elsewhere, but this file still imports v1.17.0.
Consider updating to the latest available version (v1.26.0 or newer) for better compatibility and access to newer semantic conventions.
- semconv "go.opentelemetry.io/otel/semconv/v1.17.0" + semconv "go.opentelemetry.io/otel/semconv/v1.26.0"
🧹 Nitpick comments (2)
router-tests/plugintest/tracing_test.go (2)
167-167: Remove debug print statementThis debug print statement should be removed as it's not needed in production test code and could clutter test output.
- fmt.Println("Attribute:", attr.Key, "Value:", attr.Value.AsString())
693-704: Consider making sampler configurableThe base config uses a fixed sampler value of 1.0 (100% sampling). Consider making this configurable or documenting why 100% sampling is appropriate for tests.
func getTracingBaseConfig() config.StartupConfig { return config.StartupConfig{ Telemetry: &config.Telemetry{ Tracing: &config.Tracing{ - Sampler: 1.0, + Sampler: 1.0, // 100% sampling for deterministic test results Exporters: []config.Exporter{ {}, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
router-tests/plugintest/hello/generated/service.pb.gois excluded by!**/*.pb.go,!**/generated/**router-tests/plugintest/hello/generated/service_grpc.pb.gois excluded by!**/*.pb.go,!**/generated/**
📒 Files selected for processing (2)
router-tests/plugintest/setup.go(1 hunks)router-tests/plugintest/tracing_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- router-tests/plugintest/setup.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
router-tests/plugintest/tracing_test.go (5)
498-611: Well-structured HTTP client tracing testsThe HTTP client tracing tests are comprehensive and well-structured. They properly test both disabled and enabled scenarios, verify span attributes, and validate the correct number of spans are created.
613-688: Comprehensive header propagation testThis test effectively validates that all major tracing propagation formats (TraceContext, B3, Jaeger, Baggage, Datadog) are properly implemented. The hardcoded trace ID and expected header values are appropriate for testing purposes.
708-732: Robust test setup helper functionThe
setupTracingTestfunction is well-designed with a sensible default runner implementation and flexible configuration options. The use of generics for the client type adds good type safety.
24-691: Comprehensive test coverage with good organizationThe test suite provides excellent coverage of tracing functionality:
- Negative cases: Tests when tracing should be disabled (missing config, disabled flag, etc.)
- Positive cases: Validates proper span creation with correct attributes
- Customization: Tests custom service names, versions, and span attributes
- IP anonymization: Covers all anonymization methods (disabled, redact, hash)
- HTTP client integration: Tests both disabled and enabled HTTP tracing
- Header propagation: Validates all major propagation formats
The test organization with subtests and parallel execution where appropriate demonstrates good testing practices.
However, consider adding tests for error scenarios (e.g., malformed trace headers) to ensure robust error handling.
493-494: Hashes Verified
The hardcoded SHA-256 hashes for IP addresses “127.2.2.5” and “127.3.2.5” have been confirmed to match the computed values in router-tests/plugintest/tracing_test.go (lines 493–494). No changes are needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
router-tests/plugintest/tracing_test.go (1)
12-12: Consider updating to newer semconv version.The current import uses semconv v1.17.0, which may be outdated. Consider updating to a more recent version like v1.26.0 for better compatibility and newer semantic conventions.
- semconv "go.opentelemetry.io/otel/semconv/v1.17.0" + semconv "go.opentelemetry.io/otel/semconv/v1.26.0"router-tests/router_plugin_test.go (1)
96-132: Well-structured telemetry integration test.The test effectively verifies that:
- Telemetry spans are correctly recorded during GraphQL query execution
- The response remains functionally correct
- Span filtering works properly (verifying only 1 occurrence of "query there")
The assertion for exactly 8 spans suggests this is the expected total for this query execution path.
Consider adding a comment explaining why 8 spans are expected for better test maintainability:
+ // Expected spans: 1 root query + 7 subgraph/operation spans require.Len(t, sn, 8)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
router-tests/plugintest/hello/generated/service.pb.gois excluded by!**/*.pb.go,!**/generated/**router-tests/plugintest/hello/generated/service_grpc.pb.gois excluded by!**/*.pb.go,!**/generated/**router-tests/plugintest/hello/generated/staticcheck.confis excluded by!**/generated/**
📒 Files selected for processing (3)
router-tests/plugintest/setup.go(1 hunks)router-tests/plugintest/tracing_test.go(1 hunks)router-tests/router_plugin_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- router-tests/plugintest/setup.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build-router
- GitHub Check: image_scan
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
router-tests/plugintest/tracing_test.go (5)
24-124: Excellent test organization.The test structure is well-organized with clear hierarchical naming and comprehensive coverage of different tracing scenarios (enabled/disabled, various configuration states). The use of subtests with descriptive names makes the test suite easy to understand and maintain.
126-307: Comprehensive span verification tests.The span verification tests thoroughly cover:
- Default span creation with proper attributes
- Custom service name and version configuration
- Plugin-customized span names and attributes
- Nested custom span creation
The assertions properly verify span properties, resource attributes, and custom attributes set by plugins.
309-496: Well-structured IP anonymization tests.The IP anonymization tests comprehensively cover all scenarios:
- Disabled anonymization (preserves original IPs)
- Nil configuration (preserves original IPs)
- Redaction (replaces with "[REDACTED]")
- Hashing (produces deterministic hash values)
The hardcoded hash values in lines 493-494 appear to be deterministic test expectations, which is appropriate for verification.
498-690: Excellent HTTP client tracing tests.The HTTP client tracing tests provide comprehensive coverage:
- Proper verification that tracing can be disabled/enabled
- Mock server setup for isolated HTTP testing
- Correct span count verification (base span + HTTP request spans)
- Thorough testing of header propagation with multiple formats (TraceContext, B3, Jaeger, Baggage, Datadog)
The test design isolates HTTP client functionality while verifying integration with the tracing system.
693-732: Well-designed helper functions.The helper functions provide excellent code reuse and test setup:
getTracingBaseConfig()provides a clean baseline configurationsetupTracingTest()offers flexibility with optional custom runner while maintaining consistent setup- Good abstraction that reduces code duplication across tests
The setup function properly handles service registration and client creation using the plugin test infrastructure.
router-tests/router_plugin_test.go (1)
12-12: Appropriate import addition.The import of the tracetest package is necessary for the new telemetry verification functionality and properly placed with other test-related imports.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
router-tests/plugintest/hello/generated/staticcheck.confis excluded by!**/generated/**
📒 Files selected for processing (3)
router-tests/plugintest/setup.go(1 hunks)router-tests/plugintest/tracing_test.go(1 hunks)router-tests/router_plugin_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- router-tests/plugintest/tracing_test.go
- router-tests/router_plugin_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: in go testing, t.fatal, t.failnow, t.skip* and similar methods cannot be called from goroutines othe...
Learnt from: Noroth
PR: wundergraph/cosmo#2088
File: demo/pkg/subgraphs/projects/src/main_test.go:0-0
Timestamp: 2025-07-29T08:19:55.720Z
Learning: In Go testing, t.Fatal, t.FailNow, t.Skip* and similar methods cannot be called from goroutines other than the main test goroutine, as they will cause a runtime panic. Use channels, t.Error*, or other synchronization mechanisms to communicate errors from goroutines back to the main test thread.
Applied to files:
router-tests/plugintest/setup.go
🧬 Code Graph Analysis (1)
router-tests/plugintest/setup.go (5)
router-tests/plugintest/hello/generated/service.pb.go (6)
QueryRunRequest(24-30)QueryRunRequest(45-45)QueryRunRequest(60-62)QueryRunResponse(72-78)QueryRunResponse(93-93)QueryRunResponse(108-110)router-tests/plugintest/hello/generated/service_grpc.pb.go (4)
UnimplementedHelloServiceServer(67-67)UnimplementedHelloServiceServer(69-71)UnimplementedHelloServiceServer(72-72)UnimplementedHelloServiceServer(73-73)router-plugin/config/settings.go (1)
StartupConfig(32-35)router-plugin/config/plugin_config.go (1)
RouterPluginConfig(5-13)router-plugin/setup/init.go (2)
GrpcServerInitOpts(17-20)GrpcServer(22-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
🔇 Additional comments (6)
router-tests/plugintest/setup.go (6)
1-15: Package structure and imports look good.The imports are well-organized and include all necessary dependencies for the gRPC test setup functionality.
17-24: Well-designed test helper service.The HelloService struct provides a flexible way to inject custom behavior into tests while maintaining proper gRPC interface compliance through embedding.
26-30: Clean and type-safe response struct design.The generic design with exported fields provides a clean interface for test consumers while maintaining type safety.
32-34: Appropriate buffer size for testing.The 1MB buffer size is reasonable for gRPC testing scenarios.
36-41: Well-structured test configuration.The configuration struct provides good separation of concerns and flexibility for different testing scenarios through its function fields.
44-62: Well-implemented test setup function.The function correctly integrates with the router plugin infrastructure, properly sets up in-memory gRPC connections, and provides clean resource management through the cleanup function. The integration with tracing configuration is handled appropriately.
Also applies to: 67-94
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router/pkg/grpcconnector/grpc_plugin.go (1)
100-109: Consider using uppercase for the environment variable name.While the implementation is correct, consider using
STARTUP_CONFIGinstead ofstartup_configto follow standard environment variable naming conventions.- pluginCmd.Env = append(pluginCmd.Env, fmt.Sprintf("%s=%s", "startup_config", configJson)) + pluginCmd.Env = append(pluginCmd.Env, fmt.Sprintf("%s=%s", "STARTUP_CONFIG", configJson))Note: If this change is made, ensure the corresponding environment variable name is updated in
router-plugin/plugin.gowhere it's read.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
router/pkg/grpcconnector/grpc_plugin.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
router/pkg/grpcconnector/grpc_plugin.go (5)
5-5: LGTM!The
encoding/jsonimport is correctly added and properly used for marshaling the startup configuration.
18-23: LGTM!The
StartupConfigfield is properly added to support passing tracing and telemetry configuration to plugins, which aligns with the PR objectives.
38-39: LGTM!The
startupConfigfield properly stores the startup configuration for later use during plugin initialization.
64-65: LGTM!The startup configuration is properly assigned in the constructor. The lack of validation suggests it's optional, which aligns with the PR objective of making tracing easy to activate without requiring specific configuration.
114-114: Good security practice!Setting
SkipHostEnv: trueprevents unintended environment variable leakage to the plugin process, ensuring only explicitly configured variables are passed. This is a good security practice.
SkArchon
changed the title
The goal is to allow the user to easily enable tracing without adding any specific configuration in the router plugin. Since the router plugin we have to export traces separately, but we want to make this painless and easy as possible.
In order to do this we identified two places where we record spans
We propagate two things for this
TODO: Tests
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation