feature: experimental tracing support #131
Conversation
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
WalkthroughThe codebase has been updated to introduce OpenTelemetry (OTLP) support for gRPC, focusing on trace collection and configuration. Changes include the addition of new interfaces and fields for experimental features and tracing, adjustments to gRPC server creation to accommodate tracing, and updates to configuration files and tests to support these enhancements. Changes
Assessment against linked issues
Poem
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (4)
- go.mod
- go.sum
- tests/go.mod
- tests/go.sum
Files selected for processing (7)
- common/interfaces.go (1 hunks)
- plugin.go (4 hunks)
- server.go (1 hunks)
- status.go (1 hunks)
- tests/configs/.rr-grpc-init.yaml (1 hunks)
- tests/configs/.rr-grpc-otel.yaml (1 hunks)
- tests/grpc_plugin_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- status.go
Additional comments: 17
common/interfaces.go (1)
- 24-25: The addition of the
Experimentalmethod to theConfigurerinterface is consistent with the PR's objective to introduce experimental features. However, this change will require all types that implement theConfigurerinterface to add an implementation for theExperimentalmethod, which could be a breaking change for existing code. Ensure that all implementations of this interface have been updated accordingly.plugin.go (7)
10-10: The addition of the OpenTelemetry SDK trace import aligns with the PR's objective to introduce OTLP tracing support.
35-37: The introduction of the
Tracerinterface is consistent with the PR's objective to standardize tracing behavior.49-52: The addition of
experimentalandtracerfields to thePluginstruct aligns with the PR's objective to support experimental OTLP tracing.121-121: The assignment of a new
TracerProvidertop.tracerin theInitmethod is consistent with the PR's objective to initialize tracing support.261-263: The update to the
Collectsmethod to accept aTracerinterface is consistent with the PR's objective to integrate tracing into the plugin's dependencies.122-122: The setting of the
experimentalfield based on thecfg.Experimental()method aligns with the PR's objective to conditionally enable experimental features.32-55: > 💡 NOTE
Codebase verification is a beta feature.
The
Configurerinterface has been successfully updated to include theExperimental()method as required. The interface definition in./common/interfaces.goconfirms the addition of theExperimental()method.server.go (1)
- 48-56: The addition of OpenTelemetry instrumentation to the gRPC server options is consistent with the PR objective and the provided summaries. The use of
p.experimentalto conditionally add theotelgrpc.NewServerHandlerwithp.traceras a parameter aligns with the intended experimental support for OTLP tracing. Ensure thatp.traceris properly initialized before this point and that all necessary configurations are in place for the tracer provider to function correctly.tests/configs/.rr-grpc-init.yaml (2)
11-18: The changes to the GRPC service configuration in the
.rr-grpc-init.yamlfile are consistent with the summary provided. Thelistenaddress and theprotofile have been updated as described.11-18: > 💡 NOTE
Codebase verification is a beta feature.
The GRPC service configuration changes have been successfully updated to reflect the new
listenaddress andprotofile. Additionally, the OTLP tracing support has been configured as indicated by the presence ofotelgrpc.NewServerHandlerinserver.go. The experimental features toggle is referenced intests/grpc_plugin_test.goandplugin.go, suggesting that the experimental OTLP tracing support is considered in the codebase. Tracing implementation details are found inplugin.go, which includes theTracerinterface and its usage.tests/configs/.rr-grpc-otel.yaml (3)
22-29: The
otelconfiguration section is well-defined with service details and an exporter set tostderr. However, ensure that thestderrexporter is only used for debugging purposes and that a more suitable exporter is configured for production environments.28-28: The
insecurefield is set tofalse, which suggests that security is a consideration. Verify if additional security configurations are required or if they are set elsewhere.1-29: The configuration file does not include any explicit reference to the
experimentalflag or command line option (./rr -e) that is supposed to activate the experimental OTLP tracing support. Confirm if this is intentional or if there should be a configuration option to enable the experimental mode.tests/grpc_plugin_test.go (3)
106-115: The changes to
TestGrpcInitcorrectly implement a gRPC connection and a ping operation, which aligns with the PR objective to introduce OTLP tracing support and test its functionality.185-207: The changes to
TestGrpcOtelcorrectly implement a gRPC connection, a ping operation, and verification of log entries related to OpenTelemetry, which aligns with the PR objective to introduce OTLP tracing support and test its functionality.130-134: Clarify whether the experimental mode for OTLP tracing is intended to be enabled only through configuration files or if it can also be toggled at runtime using the
./rr -ecommand as mentioned in the PR objective.
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- plugin.go (4 hunks)
- proxy/proxy.go (4 hunks)
- server.go (2 hunks)
- tests/grpc_plugin_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- server.go
- tests/grpc_plugin_test.go
Additional comments: 11
plugin.go (5)
6-15: The new imports for OpenTelemetry-related packages align with the PR's objective to introduce experimental OTLP tracing support.
37-39: The introduction of the
Tracerinterface is consistent with the PR's objective to standardize tracing operations.41-58: The addition of
experimental,tracer, andpropfields to thePluginstruct aligns with the PR's objective to support experimental OTLP tracing.124-129: The
Initmethod's updates to initialize the tracer and set the experimental flag are in line with the PR's objective to support OTLP tracing.262-268: The addition of a new dependency fitting for the
Tracerinterface in theCollectsmethod is consistent with the PR's objective to integrate tracing capabilities.proxy/proxy.go (6)
16-22: The import of
go.opentelemetry.io/otel/propagationaligns with the PR's objective to introduce OTLP tracing support.65-71: The addition of the
propfield to theProxystruct is consistent with the PR's objective to support OTLP tracing.77-84: The update to the
NewProxyfunction signature to include thepropparameter is consistent with the PR's objective to support OTLP tracing.248-254: The addition of the
prop.Injectcall within themakePayloadmethod aligns with the PR's objective to pass trace information to the PHP worker.67-71: The use of
mu *sync.RWMutexin theProxystruct and its proper locking and unlocking in theinvokemethod suggests that the changes are designed to be thread-safe.248-254: The secure use of the
prop.Injectmethod to inject trace information into thectxMDmap, which is then marshaled and sent to the PHP worker, ensures that sensitive trace information is not leaked externally.
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (4)
- go.mod
- go.sum
- tests/env/docker-compose-otel.yaml
- tests/env/otel-collector-config.yml
Files selected for processing (3)
- .github/workflows/linux.yml (2 hunks)
- tests/configs/.rr-grpc-otel.yaml (1 hunks)
- tests/grpc_plugin_test.go (1 hunks)
Additional comments: 7
.github/workflows/linux.yml (2)
- 95-103: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [78-102]
The PR objective mentions the addition of experimental OTLP tracing support, but the hunks show changes related to Docker setup and sleep commands for test environment setup. Please verify that the changes are consistent with the PR's objective.
- 95-103: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [84-97]
The steps for setting up mkcert and generating certificates are added, which is not mentioned in the PR objective or the generated summary. Ensure that these changes are intended and documented.
tests/configs/.rr-grpc-otel.yaml (2)
1-31: The summary provided does not match the changes in the hunk. The hunk shows a configuration file for a gRPC server with OTLP settings, but the summary discusses experimental OTLP tracing support in a server codebase.
1-31: The configuration settings for OTLP in the
tests/configs/.rr-grpc-otel.yamlfile appear to be correctly formatted and include necessary parameters such asservice_name,service_version,service_namespace,service_instance_id,insecure,compress,exporter, andendpoint.tests/grpc_plugin_test.go (3)
122-209: The changes in the
TestGrpcOtelfunction align with the PR objective of adding experimental OTLP tracing support. The test now includes a gRPC connection setup, a ping operation, and verification of log entries related to OpenTelemetry, which suggests that the OTLP tracing functionality is being tested as expected.106-120: The updates to the
TestGrpcInitfunction, which include establishing a gRPC connection and performing a ping operation, are correctly reflected in the summary.203-208: The checks for specific strings in the HTTP response body are appropriate for validating the presence of OTLP trace information in the
TestGrpcOtelfunction.
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (15)
- .github/workflows/linux.yml (2 hunks)
- tests/configs/.rr-grpc-init-duplicate-2.yaml (1 hunks)
- tests/configs/.rr-grpc-init-duplicate.yaml (1 hunks)
- tests/configs/.rr-grpc-init-multiple.yaml (1 hunks)
- tests/configs/.rr-grpc-init.yaml (2 hunks)
- tests/configs/.rr-grpc-metrics.yaml (1 hunks)
- tests/configs/.rr-grpc-otel.yaml (1 hunks)
- tests/configs/.rr-grpc-rq-exception.yaml (1 hunks)
- tests/configs/.rr-grpc-rq-issue1193.yaml (1 hunks)
- tests/configs/.rr-grpc-rq-multiple.yaml (1 hunks)
- tests/configs/.rr-grpc-rq-otlp.yaml (3 hunks)
- tests/configs/.rr-grpc-rq-tls-rootca.yaml (1 hunks)
- tests/configs/.rr-grpc-rq-tls.yaml (1 hunks)
- tests/configs/.rr-grpc-rq.yaml (1 hunks)
- tests/grpc_plugin_test.go (3 hunks)
Files skipped from review due to trivial changes (2)
- tests/configs/.rr-grpc-rq-issue1193.yaml
- tests/configs/.rr-grpc-rq.yaml
Files skipped from review as they are similar to previous changes (2)
- .github/workflows/linux.yml
- tests/configs/.rr-grpc-init.yaml
Additional comments: 16
tests/configs/.rr-grpc-init-duplicate-2.yaml (1)
- 35-35: The change from an integer to a string with a time unit for
destroy_timeoutis consistent with the PR objective to standardize time units in configuration settings.tests/configs/.rr-grpc-init-duplicate.yaml (1)
- 59-62: The change from an integer value to a string with a time unit for
destroy_timeoutis consistent with the PR objective to standardize time units in configuration settings.tests/configs/.rr-grpc-init-multiple.yaml (1)
- 32-35: The change to
destroy_timeoutfrom60to60saligns with the PR objective to standardize time units in configuration settings. This should ensure consistency and clarity in configuration parameters.tests/configs/.rr-grpc-metrics.yaml (1)
- 33-36: The changes to the
destroy_timeoutparameter from an integer to a string with a time unit are consistent with the PR objective of standardizing time units in configuration settings.tests/configs/.rr-grpc-otel.yaml (1)
- 1-35: The configuration file
.rr-grpc-otel.yamlis well-formed and aligns with the PR objective of adding experimental OTLP tracing support. Theotelsection is correctly configured to enable tracing with appropriate service identification and endpoint settings. Ensure that theinsecureflag is intentional and only used in development or testing environments.tests/configs/.rr-grpc-rq-exception.yaml (1)
- 62-65: The change from an integer to a string with a time unit for
destroy_timeoutis consistent with the PR objective to standardize time units in configuration settings.tests/configs/.rr-grpc-rq-multiple.yaml (1)
- 63-66: The changes to the
destroy_timeoutparameter from an integer to a string with a time unit are consistent with the PR objective to standardize time units in configuration settings.tests/configs/.rr-grpc-rq-otlp.yaml (4)
- 29-42: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [16-42]
The summary mentions the removal of
max_connection_idle,max_connection_age, andmax_connection_age_gracesettings, but these are not shown in the provided hunks. Please verify if these settings were indeed removed and if the summary needs to be updated.
12-13: The change in logging level from "error" to "debug" is correctly reflected in the hunk.
32-32: The summary states that
destroy_timeoutwas modified from 60 to 60 seconds, but the hunk shows it already includes a time unit. Please verify if this is a change from a previous value without a time unit or if the summary needs to be updated.38-42: The addition of OpenTelemetry resource configurations is correctly reflected in the hunk.
tests/configs/.rr-grpc-rq-tls-rootca.yaml (1)
- 67-70: The changes to the
destroy_timeoutparameter from an integer to a string with a time unit are consistent with the PR objective to standardize time units in configuration settings.tests/configs/.rr-grpc-rq-tls.yaml (1)
- 67-68: The change from an integer value to a string with a time unit for
destroy_timeoutis consistent with the PR objective to standardize time units in configuration settings.tests/grpc_plugin_test.go (3)
192-192: The previous comment about replacing
time.Sleepwith a more reliable synchronization mechanism is still valid.1127-1136: The configuration for the
Test_GrpcRqOtlptest function has been updated to enable experimental features, which aligns with the PR objective to introduce experimental OTLP tracing support.1203-1206: The test captures stderr to a buffer to check for specific log entries related to OTLP tracing. This is a good approach to validate that the tracing functionality is working as expected.
Reason for This PR
closes: roadrunner-server/roadrunner#1782
Description of Changes
./rr -eto activate: linkLicense Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Documentation
Refactor
Tests
Please note that some internal code changes and file names have been omitted for confidentiality.