feat: support for both HTTP and gRPC protocols in meter/logging/tracing#1490
feat: support for both HTTP and gRPC protocols in meter/logging/tracing#1490tolgaozen merged 3 commits intoPermify:masterfrom vikashsprem:feat/support-grpc
Conversation
|
Someone is attempting to deploy a commit to the permify Team on Vercel. A member of the Team first needs to authorize it. |
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ServeFunction
participant ExporterFactory
participant NewOTLP
User->>ServeFunction: Call with protocol
ServeFunction->>ExporterFactory: Create LogExporter with protocol
ExporterFactory->>NewOTLP: Create using protocol
NewOTLP-->>ExporterFactory: Return configured exporter
ExporterFactory-->>ServeFunction: Return exporter
ServeFunction-->>User: Return success
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
💵 To receive payouts, sign up on Algora, link your Github account and connect with Stripe. |
…g/tracing exporters
|
@tolgaozen Please review this PR. Thank you. |
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- internal/config/config.go (3 hunks)
- pkg/cmd/serve.go (3 hunks)
- pkg/telemetry/logexporters/factory.go (1 hunks)
- pkg/telemetry/logexporters/otlp.go (1 hunks)
- pkg/telemetry/meterexporters/factory.go (1 hunks)
- pkg/telemetry/meterexporters/otlp.go (1 hunks)
- pkg/telemetry/tracerexporters/factory.go (1 hunks)
- pkg/telemetry/tracerexporters/otlp.go (1 hunks)
Additional comments not posted (15)
pkg/telemetry/logexporters/otlp.go (3)
15-36: LGTM: HTTP protocol implementation.The HTTP case is well-implemented, correctly handling URL path and insecure connections.
38-54: LGTM: gRPC protocol implementation.The gRPC case is well-implemented, correctly handling insecure connections.
56-58: LGTM: Default case for unsupported protocols.The default case correctly handles unsupported protocols by returning an error.
pkg/telemetry/tracerexporters/otlp.go (3)
16-38: LGTM: HTTP protocol implementation.The HTTP case is well-implemented, correctly handling headers, URL path, and insecure connections.
40-60: LGTM: gRPC protocol implementation.The gRPC case is well-implemented, correctly handling headers and secure connections.
62-64: LGTM: Default case for unsupported protocols.The default case correctly handles unsupported protocols by returning an error.
pkg/telemetry/meterexporters/otlp.go (3)
16-39: LGTM: HTTP protocol implementation.The HTTP case is well-implemented, correctly handling headers, URL path, and insecure connections.
41-58: LGTM: gRPC protocol implementation.The gRPC case is well-implemented, correctly handling headers and secure connections.
60-61: LGTM: Default case for unsupported protocols.The default case correctly handles unsupported protocols by returning an error.
internal/config/config.go (3)
98-98: LGTM! Protocol field added to LogExporter.The addition of the
Protocolfield to theLogExporterstruct is clear and consistent with the intended functionality.
109-109: LGTM! Protocol field added to Tracer.The addition of the
Protocolfield to theTracerstruct is consistent and well-documented.
121-121: LGTM! Protocol field added to Meter.The addition of the
Protocolfield to theMeterstruct is consistent and well-documented.pkg/cmd/serve.go (3)
203-203: LGTM! Protocol field used in logger configuration.The use of the
Protocolfield in the logger configuration is consistent and aligns with the intended functionality.
297-297: LGTM! Protocol field used in tracer configuration.The use of the
Protocolfield in the tracer configuration is consistent and aligns with the intended functionality.
349-349: LGTM! Protocol field used in meter configuration.The use of the
Protocolfield in the meter configuration is consistent and aligns with the intended functionality.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- internal/config/config.go (3 hunks)
- pkg/cmd/serve.go (3 hunks)
- pkg/telemetry/logexporters/factory.go (1 hunks)
- pkg/telemetry/logexporters/otlp.go (1 hunks)
- pkg/telemetry/meterexporters/factory.go (1 hunks)
- pkg/telemetry/meterexporters/otlp.go (1 hunks)
- pkg/telemetry/tracerexporters/factory.go (1 hunks)
- pkg/telemetry/tracerexporters/otlp.go (1 hunks)
Additional comments not posted (9)
pkg/telemetry/logexporters/otlp.go (1)
13-58: LGTM! But verify the function usage in the codebase.The changes are well-structured and enhance the flexibility of the
NewOTLPfunction by supporting both HTTP and gRPC protocols. The error handling for unsupported protocols is a good addition.Ensure that all function calls to
NewOTLPcorrectly pass the newprotocolparameter.Run the following script to verify the function usage:
Verification successful
Function Usage Verified: All Calls to
NewOTLPPass the Protocol Parameter CorrectlyThe function
NewOTLPis used in various parts of the codebase, and all instances correctly pass theprotocolparameter as required. The calls specify the protocol explicitly, ensuring the function operates as intended.
pkg/telemetry/meterexporters/factory.gopkg/telemetry/tracerexporters/factory.gopkg/telemetry/logexporters/factory.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `NewOTLP` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'NewOTLP'Length of output: 4634
pkg/telemetry/tracerexporters/otlp.go (1)
14-64: LGTM! But verify the function usage in the codebase.The changes are consistent and enhance the flexibility of the
NewOTLPfunction by supporting both HTTP and gRPC protocols. The error handling for unsupported protocols is a good addition.Ensure that all function calls to
NewOTLPcorrectly pass the newprotocolparameter.Run the following script to verify the function usage:
Verification successful
Function Usage Verified: All
NewOTLPCalls Updated with Protocol ParameterThe
NewOTLPfunction calls across the codebase correctly pass the newprotocolparameter, ensuring consistency with the updated function signature.
pkg/telemetry/tracerexporters/factory.go: CallsNewOTLPwith theprotocolparameter.pkg/telemetry/meterexporters/factory.go: CallsNewOTLPwith theprotocolparameter.pkg/telemetry/logexporters/factory.go: CallsNewOTLPwith theprotocolparameter.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `NewOTLP` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'NewOTLP'Length of output: 4634
pkg/telemetry/meterexporters/otlp.go (1)
14-61: LGTM! But verify the function usage in the codebase.The changes are consistent and enhance the flexibility of the
NewOTLPfunction by supporting both HTTP and gRPC protocols. The error handling for unsupported protocols is a good addition.Ensure that all function calls to
NewOTLPcorrectly pass the newprotocolparameter.Run the following script to verify the function usage:
Verification successful
Function usage verified successfully. All calls to the
NewOTLPfunction correctly pass theprotocolparameter as intended. The changes are consistent across the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `NewOTLP` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'NewOTLP'Length of output: 4634
internal/config/config.go (3)
98-98: Addition ofProtocolfield inLogstruct.The
Protocolfield has been added to allow specification of the communication protocol for the log exporter. This change aligns with the PR objectives to support both HTTP and gRPC protocols.
109-109: Addition ofProtocolfield inTracerstruct.The
Protocolfield has been added to allow specification of the communication protocol for the tracing exporter. This change aligns with the PR objectives to support both HTTP and gRPC protocols.
121-121: Addition ofProtocolfield inMeterstruct.The
Protocolfield has been added to allow specification of the communication protocol for the metrics exporter. This change aligns with the PR objectives to support both HTTP and gRPC protocols.pkg/cmd/serve.go (3)
203-203: Addition ofProtocolparameter in logger initialization.The
Protocolparameter has been added to thelogexporters.ExporterFactorycall, enhancing the flexibility of the logging configuration by allowing different protocols.
297-297: Addition ofProtocolparameter in tracer initialization.The
Protocolparameter has been added to thetracerexporters.ExporterFactorycall, enhancing the flexibility of the tracing configuration by allowing different protocols.
349-349: Addition ofProtocolparameter in meter initialization.The
Protocolparameter has been added to themeterexporters.ExporterFactorycall, enhancing the flexibility of the metering configuration by allowing different protocols.
| func ExporterFactory(name, endpoint string, insecure bool, urlpath string, headers map[string]string, protocol string) (metric.Exporter, error) { | ||
| switch name { | ||
| case "otlp", "otlp-http": | ||
| return NewOTLP(endpoint, insecure, urlpath, headers) | ||
| return NewOTLP(endpoint, insecure, urlpath, headers, "http") | ||
| case "otlp-grpc": | ||
| return NewOTLPGrpc(endpoint, insecure, headers) | ||
| return NewOTLP(endpoint, insecure, urlpath, headers, "grpc") |
There was a problem hiding this comment.
Utilize the protocol parameter directly.
The protocol parameter is added to the function signature but is not used in the logic. Consider using the protocol parameter directly instead of hardcoding the values.
Apply this diff to use the protocol parameter:
switch name {
case "otlp", "otlp-http":
- return NewOTLP(endpoint, insecure, urlpath, headers, "http")
case "otlp-grpc":
- return NewOTLP(endpoint, insecure, urlpath, headers, "grpc")
+ return NewOTLP(endpoint, insecure, urlpath, headers, protocol)Committable suggestion was skipped due to low confidence.
| func ExporterFactory(name, endpoint string, insecure bool, urlpath string, headers map[string]string, protocol string) (*otlplogs.Exporter, error) { | ||
| switch name { | ||
| case "otlp", "otlp-http": | ||
| return NewOTLP(endpoint, insecure, urlpath, headers) | ||
| return NewOTLP(endpoint, insecure, urlpath, headers, "http") | ||
| case "otlp-grpc": | ||
| return nil, fmt.Errorf("%s log exporter is unsupported", name) | ||
| return NewOTLP(endpoint, insecure, urlpath, headers, "grpc") |
There was a problem hiding this comment.
Utilize the protocol parameter directly.
The protocol parameter is added to the function signature but is not used in the logic. Consider using the protocol parameter directly instead of hardcoding the values.
Apply this diff to use the protocol parameter:
switch name {
case "otlp", "otlp-http":
- return NewOTLP(endpoint, insecure, urlpath, headers, "http")
case "otlp-grpc":
- return NewOTLP(endpoint, insecure, urlpath, headers, "grpc")
+ return NewOTLP(endpoint, insecure, urlpath, headers, protocol)Committable suggestion was skipped due to low confidence.
| func ExporterFactory(name, url string, insecure bool, urlpath string, headers map[string]string, protocol string) (trace.SpanExporter, error) { | ||
| switch name { | ||
| case "zipkin": | ||
| return NewZipkin(url) | ||
| case "jaeger": | ||
| return NewJaegar(url) | ||
| case "otlp", "otlp-http": | ||
| return NewOTLP(url, insecure, urlpath, headers) | ||
| return NewOTLP(url, insecure, urlpath, headers, "http") | ||
| case "otlp-grpc": | ||
| return NewOTLPGrpc(url, insecure, headers) | ||
| return NewOTLP(url, insecure, urlpath, headers, "grpc") |
There was a problem hiding this comment.
Utilize the protocol parameter directly.
The protocol parameter is added to the function signature but is not used in the logic. Consider using the protocol parameter directly instead of hardcoding the values.
Apply this diff to use the protocol parameter:
case "otlp", "otlp-http":
- return NewOTLP(url, insecure, urlpath, headers, "http")
case "otlp-grpc":
- return NewOTLP(url, insecure, urlpath, headers, "grpc")
+ return NewOTLP(url, insecure, urlpath, headers, protocol)Committable suggestion was skipped due to low confidence.
|
Now I have fixed the code inconsistency reviewed by coderabbitai |
|
@tolgaozen , Please review and let me know if any changes are needed. |
|
hello @vikashsprem , thank you for your contribution 🚀 |
/claim #1090
/close #1090
Add support for both HTTP and gRPC protocols in meter/logging/tracing in meter/logging/tracing exports
Summary by CodeRabbit
New Features
Protocolconfiguration field for log, tracing, and metrics exporters, allowing users to specify communication protocols (HTTP or gRPC) for enhanced data transmission.protocolparameter, increasing flexibility in configuring log, tracer, and meter exporters.Bug Fixes
Documentation