[SLT-141] feat(metrics): multiple exports #3099
Conversation
WalkthroughThe changes introduce a multi-exporter for OpenTelemetry trace data, enabling simultaneous span exports to multiple OTLP exporters. Enhancements to the metrics handler include support for multiple OTLP exporters and improved client creation logic. Documentation updates clarify the use of new environment variables for configuring exporters. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OTLPHandler
participant MultiExporter
participant Exporter1
participant Exporter2
User->>OTLPHandler: Send Trace Data
OTLPHandler->>MultiExporter: Export Spans
MultiExporter->>Exporter1: Export Spans
MultiExporter->>Exporter2: Export Spans
Exporter1-->>MultiExporter: Success/Failure
Exporter2-->>MultiExporter: Success/Failure
MultiExporter-->>OTLPHandler: Export Result
OTLPHandler-->>User: Acknowledge
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- core/metrics/multiexporter.go (1 hunks)
- core/metrics/otlp.go (5 hunks)
- core/metrics/rookout.go (1 hunks)
Additional comments not posted (11)
core/metrics/rookout.go (2)
1-1: Verify the reason for the build constraint and its impact.The build constraint restricts the compatibility of the code to Go versions 1.16 through 1.22, excluding 1.23. This change could potentially impact users who may be using Go 1.23 or later.
Please provide more context on the reason for introducing this build constraint. Also, consider the impact on users and document it appropriately.
6-7: LGTM!The change in the import section improves clarity.
core/metrics/multiexporter.go (4)
11-13: LGTM!The code changes are approved.
18-22: LGTM!The code changes are approved.
25-33: Verify the error handling behavior.The current implementation returns an error immediately if any of the exporters fail to export spans. This may not be the desired behavior in all cases, as it prevents the remaining exporters from receiving the spans.
Consider the following alternative approaches:
- Log the error and continue exporting to the remaining exporters.
- Accumulate the errors and return them as a single error after attempting to export to all the exporters.
Discuss with the team and choose the approach that aligns with the project's error handling strategy.
36-44: Verify the error handling behavior.The current implementation returns an error immediately if any of the exporters fail to shut down. This may not be the desired behavior in all cases, as it prevents the remaining exporters from being properly shut down.
Consider the following alternative approaches:
- Log the error and continue shutting down the remaining exporters.
- Accumulate the errors and return them as a single error after attempting to shut down all the exporters.
Discuss with the team and choose the approach that aligns with the project's error handling strategy.
core/metrics/otlp.go (5)
31-39: Refactoring improves modularity and error handling.The refactoring of the
Startmethod to use thebuildClientFromTransportfunction for creating the OTLP clients is a great improvement. It enhances the modularity and readability of the code by abstracting the client creation logic into a separate function. The addition of a secondary client allows for more flexible data exporting configurations.Moreover, the improved error handling, which returns specific errors when client creation fails, enhances the robustness of the code by providing more informative error messages.
52-62: MultiExporter enhances flexibility in data exporting.The introduction of the
MultiExporter, which combines the primary and secondary OTLP exporters, is a valuable addition. It allows for simultaneous span exports to multiple OTLP exporters, providing flexibility in data exporting configurations.Furthermore, the initialization of the
baseHandlerwith theMultiExporterand the configuration of maximum queue size and export batch size parameters improves the control over the exporting behavior, enabling fine-tuning of the exporting process.
108-109: New constant for secondary OTLP transport improves maintainability.The addition of the
otlpTransportEnvSecondaryconstant for the secondary OTLP transport environment variable is a good practice. It provides a clear and maintainable way to reference the secondary transport, making the code more readable and less prone to errors.Keeping the existing
otlpTransportEnvconstant unchanged ensures backward compatibility.
120-129: New function improves code modularity and error handling.The introduction of the
buildClientFromTransportfunction is a great addition to the codebase. It abstracts the client creation logic based on the transport type, improving code modularity and reusability. The function takes anotlpTransportTypeas input and returns anotlptrace.Clientand an error, providing a clear and consistent interface.The use of a switch statement to handle different transport types makes the code more readable and maintainable. Additionally, returning an error for unknown transport types enhances error handling and prevents unexpected behavior.
145-148: New constants for default values improve readability and maintainability.The introduction of the
defaultMaxQueueSizeanddefaultMaxExportBatchconstants is a good practice. It provides clear and maintainable default values for the maximum queue size and export batch size, respectively.Using constants instead of hardcoded values improves code readability and makes it easier to modify these values in the future if needed. It also ensures consistency throughout the codebase.
Deploying sanguine-fe with
|
| Latest commit: |
0e3a5ae
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3f23bdc3.sanguine-fe.pages.dev |
| Branch Preview URL: | https://multiple-exports.sanguine-fe.pages.dev |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3099 +/- ##
===================================================
- Coverage 33.38834% 32.90317% -0.48517%
===================================================
Files 343 534 +191
Lines 20603 33398 +12795
Branches 82 82
===================================================
+ Hits 6879 10989 +4110
- Misses 13147 21431 +8284
- Partials 577 978 +401
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- core/metrics/otlp.go (5 hunks)
Additional comments not posted (5)
core/metrics/otlp.go (5)
31-39: LGTM!The changes improve error handling and abstract the client creation logic.
41-47: LGTM!The changes introduce support for a secondary OTLP exporter and handle errors appropriately.
55-60: LGTM!The changes integrate the secondary exporter into the multi-exporter setup and handle errors appropriately.
62-70: LGTM!The changes integrate the multi-exporter into the
baseHandlerand provide clearer configuration options.
128-137: LGTM!The
buildClientFromTransportfunction improves the modularity and robustness of the client creation process.
golangisfun123
changed the title
core/metrics/otlp.go
Outdated
| @@ -101,6 +125,17 @@ const ( | |||
| otlpTransportGRPC // grpc | |||
| ) | |||
|
|
|||
| func buildClientFromTransport(transport otlpTransportType) (otlptrace.Client, error) { | |||
There was a problem hiding this comment.
Relevant docs:
exporters general doc
exporter envs these are used for construction of otlp client
sent a loom but tl;dr, at the moment you don't actually allow different urls b/c any otlptracehttp.NewClient(), or otlptracehttp.NewClient(), is going to use the defualt enviornment variable name
Seperately, make sure you add new enviornment variables to metrics/README.md
core/metrics/otlp.go
Outdated
| return fmt.Errorf("could not create client: %w", err) | ||
| } | ||
|
|
||
| secondaryClient, err := buildClientFromTransport( |
There was a problem hiding this comment.
we should be able to tolerate cases where user doesn't define a secondary transport
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- core/metrics/otlp.go (5 hunks)
Additional context used
GitHub Check: codecov/patch
core/metrics/otlp.go
[warning] 39-40: core/metrics/otlp.go#L39-L40
Added lines #L39 - L40 were not covered by tests
[warning] 49-49: core/metrics/otlp.go#L49
Added line #L49 was not covered by tests
[warning] 59-60: core/metrics/otlp.go#L59-L60
Added lines #L59 - L60 were not covered by tests
[warning] 132-132: core/metrics/otlp.go#L132
Added line #L132 was not covered by tests
[warning] 136-137: core/metrics/otlp.go#L136-L137
Added lines #L136 - L137 were not covered by tests
Additional comments not posted (4)
core/metrics/otlp.go (4)
31-49: Verify handling of missing secondary transport.The initialization of
clientandsecondaryClientuses environment variables with sensible defaults. The error handling is robust, but ensure that the application gracefully handles cases where the secondary transport is not defined, as suggested in a previous review.Consider testing or documenting the behavior when
OTEL_EXPORTER_OTLP_TRANSPORT_SECONDARYis not set.Tools
GitHub Check: codecov/patch
[warning] 39-40: core/metrics/otlp.go#L39-L40
Added lines #L39 - L40 were not covered by tests
[warning] 49-49: core/metrics/otlp.go#L49
Added line #L49 was not covered by tests
62-72: Approve the creation and integration ofmultiExporter.The
multiExporteris effectively created and integrated with thebaseHandler, utilizing both the primary and secondary exporters. This setup enhances the modularity and robustness of the metrics handling.
156-158: Approve the addition of export configuration constants.The introduction of
defaultMaxQueueSizeanddefaultMaxExportBatchprovides clear and sensible defaults for managing the exporter's performance and resource usage.
154-158: Approve the graceful shutdown handling.The
handleShutdownfunction is well-implemented, using a derived context with a timeout to ensure a graceful shutdown. The approach of waiting a bit before shutting down to allow the last spans to be sent is prudent.
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range, codebase verification and nitpick comments (1)
core/metrics/README.md (1)
15-21: Typographical errors in the documentation.There are several instances of the word "environment" being misspelled as "enviornment" throughout the document. Consistency and correctness in documentation are crucial for professional quality and user comprehension.
Please correct all instances of this typographical error to maintain the quality of the documentation.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- core/metrics/README.md (1 hunks)
- core/metrics/otlp.go (5 hunks)
Additional context used
GitHub Check: codecov/patch
core/metrics/otlp.go
[warning] 67-67: core/metrics/otlp.go#L67
Added line #L67 was not covered by tests
[warning] 138-139: core/metrics/otlp.go#L138-L139
Added lines #L138 - L139 were not covered by tests
[warning] 142-143: core/metrics/otlp.go#L142-L143
Added lines #L142 - L143 were not covered by tests
core/metrics/README.md
Outdated
| We do our best to support enviornment variables specified in the [Otel Spec](https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/) and have added a few of our own. This was to allow for multiple exporter backends for traces, as otel clients only allow for one URL. The relevant multi exporter code is in `multiexporter.go`, and simply wraps multiple otel clients. | ||
|
|
||
| The additional environment variables to note are: | ||
| | Enviornment Variable | Description | Default | | ||
| |------------------------------------------|-------------------------------------------|---------| | ||
| | `OTEL_EXPORTER_OTLP_TRANSPORT_PRIMARY` | Primary exporter URL to send traces to. | `true` | | ||
| | `OTEL_EXPORTER_OTLP_TRANSPORT_SECONDARY` | Secondary exporter URL to send traces to. | `8080` | |
There was a problem hiding this comment.
Clarify the default values for new environment variables.
The documentation introduces two new environment variables, OTEL_EXPORTER_OTLP_TRANSPORT_PRIMARY and OTEL_EXPORTER_OTLP_TRANSPORT_SECONDARY. However, the default values listed seem incorrect:
OTEL_EXPORTER_OTLP_TRANSPORT_PRIMARYis listed with a default oftrue, which is unusual for a URL.OTEL_EXPORTER_OTLP_TRANSPORT_SECONDARYis listed with a default of8080, which typically represents a port, not a URL.
Please revise the default values to reflect appropriate URLs or clarify if these are placeholders.
Enhance the explanation of the multi-exporter functionality.
The section explaining the multi-exporter functionality could benefit from a more detailed explanation or example. The current text mentions that the multiexporter.go file "simply wraps multiple otel clients," but it does not explain how this is achieved or how users can configure or utilize this functionality effectively.
Consider adding a code snippet or a more detailed step-by-step guide on setting up multiple exporters using the provided environment variables.
Inconsistent information regarding support for profiling.
The table mentions that OTLP does not support profiling directly but can do so through Pyroscope by specifying the PYROSCOPE_ENDPOINT environment variable. However, there is no further explanation or reference to Pyroscope in the OTLP section, which might confuse users about how to set up profiling support.
It would be beneficial to include a brief explanation or cross-reference to the Pyroscope section within the OTLP documentation to clarify how profiling can be enabled.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- core/metrics/otlp.go (5 hunks)
Additional comments not posted (4)
core/metrics/otlp.go (4)
31-49: Refactor client creation logic to handle secondary transport conditionally.The current implementation always attempts to create a secondary client, even if the environment variable for the secondary transport is not set. This could lead to unnecessary error handling or exceptions when the secondary transport is not intended to be used.
Consider refactoring the logic to check if the secondary transport environment variable is set before attempting to create the secondary client. This approach aligns with the previous reviewer's suggestion to tolerate cases where the user doesn't define a secondary transport.
Here's a proposed refactor to conditionally handle the secondary client creation:
- secondaryClient, err = getClient(otlpTransportEnvSecondary) - if err != nil { - return fmt.Errorf("could not create secondary client: %w", err) - } + if core.GetEnv(otlpTransportEnvSecondary, "") != "" { + secondaryClient, err = getClient(otlpTransportEnvSecondary) + if err != nil { + return fmt.Errorf("could not create secondary client: %w", err) + } + }This comment has been previously made and is still valid.
57-67: Ensure test coverage for multi-exporter logic.The introduction of the multi-exporter logic is a significant enhancement. However, static analysis indicates that the line where the multi-exporter is created with only the primary exporter (line 67) is not covered by tests.
It's crucial to ensure that both scenarios (with and without the secondary exporter) are covered by unit tests to verify that the multi-exporter behaves as expected in all configurations.
Consider adding unit tests to cover these scenarios to ensure robust error handling and functionality verification.
This comment has been previously made and is still valid.Tools
GitHub Check: codecov/patch
[warning] 67-67: core/metrics/otlp.go#L67
Added line #L67 was not covered by tests
123-125: Clarify the use of new environment variables in documentation.The introduction of new constants for environment variables (
otlpTransportEnvPrimaryandotlpTransportEnvSecondary) is a key part of this feature. However, it's important to ensure that these new variables are well-documented.As per the previous reviewer's suggestion, add these environment variables to the
metrics/README.mdto ensure that users are aware of how to configure the primary and secondary transports.Would you like me to help update the documentation or open a GitHub issue to track this task?
This comment has been previously made and is still valid.
136-149: Improve error handling inbuildClientFromTransport.The function
buildClientFromTransportis crucial for creating clients based on the transport type. However, static analysis indicates that the error handling for unknown transport types (lines 142-143) is not covered by tests.This lack of coverage could lead to untested paths in production, potentially resulting in runtime errors if an unsupported transport type is accidentally used.
Enhance the robustness of this function by adding unit tests to cover these error scenarios. Additionally, consider logging or monitoring these errors to aid in troubleshooting and maintaining the system's stability.
This comment has been previously made and is still valid.Tools
GitHub Check: codecov/patch
[warning] 138-139: core/metrics/otlp.go#L138-L139
Added lines #L138 - L139 were not covered by tests[warning] 142-143: core/metrics/otlp.go#L142-L143
Added lines #L142 - L143 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 7
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- core/metrics/multiexporter.go (1 hunks)
- core/metrics/otlp.go (5 hunks)
Additional context used
GitHub Check: codecov/patch
core/metrics/multiexporter.go
[warning] 31-36: core/metrics/multiexporter.go#L31-L36
Added lines #L31 - L36 were not covered by tests
[warning] 38-38: core/metrics/multiexporter.go#L38
Added line #L38 was not covered by tests
[warning] 46-47: core/metrics/multiexporter.go#L46-L47
Added lines #L46 - L47 were not covered by testscore/metrics/otlp.go
[warning] 35-38: core/metrics/otlp.go#L35-L38
Added lines #L35 - L38 were not covered by tests
[warning] 43-44: core/metrics/otlp.go#L43-L44
Added lines #L43 - L44 were not covered by tests
[warning] 48-49: core/metrics/otlp.go#L48-L49
Added lines #L48 - L49 were not covered by tests
[warning] 62-63: core/metrics/otlp.go#L62-L63
Added lines #L62 - L63 were not covered by tests
[warning] 65-65: core/metrics/otlp.go#L65
Added line #L65 was not covered by tests
[warning] 152-153: core/metrics/otlp.go#L152-L153
Added lines #L152 - L153 were not covered by tests
[warning] 156-157: core/metrics/otlp.go#L156-L157
Added lines #L156 - L157 were not covered by tests
Additional comments not posted (4)
core/metrics/multiexporter.go (4)
11-15: Interface Definition Review: MultiExporterThe
MultiExporterinterface is well-defined, extendingtracesdk.SpanExporterand including anAddExportermethod. This design supports the dynamic addition of exporters, which is crucial for the flexibility of exporting to multiple backends.
17-19: Struct Definition Review: multiExporterThe
multiExporterstruct is simple and effective, holding a slice of*otlptrace.Exporter. This structure supports the intended functionality of managing multiple exporters.
21-28: Function Implementation Review: NewMultiExporterThe
NewMultiExporterfunction is implemented correctly to instantiatemultiExporterwith a variable number ofotlptrace.Exporterinstances. This flexibility allows easy setup of multiple export targets.
52-55: Method Implementation Review: AddExporterThe
AddExportermethod correctly appends a new exporter to the slice. This method is straightforward and effectively allows the dynamic addition of exporters at runtime.
core/metrics/multiexporter.go
Outdated
| func (m *multiExporter) ExportSpans(ctx context.Context, ss []tracesdk.ReadOnlySpan) error { | ||
| for _, exporter := range m.exporters { | ||
| err := exporter.ExportSpans(ctx, ss) | ||
| if err != nil { | ||
| return fmt.Errorf("could not export spans: %w", err) | ||
| } |
There was a problem hiding this comment.
Test Coverage Issue
The static analysis tool has flagged several lines in the ExportSpans and Shutdown methods as not covered by tests. This lack of coverage could lead to undetected issues in production.
It is crucial to add unit tests covering these methods to ensure their functionality under various scenarios, especially error handling.
Would you like assistance in writing these tests or perhaps a suggestion on how to structure them?
Also applies to: 38-38, 46-47
Tools
GitHub Check: codecov/patch
[warning] 31-36: core/metrics/multiexporter.go#L31-L36
Added lines #L31 - L36 were not covered by tests
There was a problem hiding this comment.
There was a problem hiding this comment.
@trajan0x, I've reviewed the changes, and the test coverage issue has been resolved. The ExportSpans and Shutdown methods are now covered in the TestMultiExporter function, ensuring that their functionality is tested. Great work on addressing this!
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Fetch the diff file to inspect the changes made by the user.
gh pr diff 3099 --path core/metrics/multiexporter_test.go
Length of output: 411
Script:
#!/bin/bash
# List the files changed in the pull request to confirm if core/metrics/multiexporter_test.go is present.
gh pr diff 3099 --name-only
Length of output: 162
Script:
#!/bin/bash
# Fetch the diff for core/metrics/multiexporter_test.go to inspect the changes.
gh pr diff 3099 -- core/metrics/multiexporter_test.go
Length of output: 92
Script:
#!/bin/bash
# Fetch the entire diff for the pull request and filter for core/metrics/multiexporter_test.go.
gh pr diff 3099 | awk '/core\/metrics\/multiexporter_test.go/,/^diff/'
Length of output: 1604
core/metrics/multiexporter.go
Outdated
| // ExportSpans exports a batch of spans. | ||
| func (m *multiExporter) ExportSpans(ctx context.Context, ss []tracesdk.ReadOnlySpan) error { | ||
| for _, exporter := range m.exporters { | ||
| err := exporter.ExportSpans(ctx, ss) | ||
| if err != nil { | ||
| return fmt.Errorf("could not export spans: %w", err) | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Method Implementation Review: ExportSpans
The ExportSpans method iterates over each exporter and attempts to export spans. If an error occurs during export, it immediately returns the error, halting further exports. This could be reconsidered to attempt all exports before returning an error, which would ensure that a failure in one exporter doesn't prevent others from receiving data.
Consider modifying the error handling to allow all exporters a chance to receive spans before returning an error. This could be achieved by collecting all errors and returning a combined error if necessary.
- if err != nil {
- return fmt.Errorf("could not export spans: %w", err)
- }
+ if err != nil {
+ // Collect errors from all exporters
+ allErrors = append(allErrors, err)
+ }
+ }
+ if len(allErrors) > 0 {
+ return fmt.Errorf("could not export spans to some exporters: %v", allErrors)
+ }Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: codecov/patch
[warning] 31-36: core/metrics/multiexporter.go#L31-L36
Added lines #L31 - L36 were not covered by tests
[warning] 38-38: core/metrics/multiexporter.go#L38
Added line #L38 was not covered by tests
There was a problem hiding this comment.
@golangisfun123 thoughts in desirable behavior?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
core/metrics/multiexporter.go
Outdated
| // Shutdown notifies the exporter of a pending halt to operations. | ||
| func (m *multiExporter) Shutdown(ctx context.Context) error { | ||
| for _, exporter := range m.exporters { | ||
| err := exporter.Shutdown(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("could not stop exporter: %w", err) | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Method Implementation Review: Shutdown
Similar to ExportSpans, the Shutdown method stops at the first error encountered. This could potentially leave some exporters running if one fails to shut down properly.
Consider implementing a similar error aggregation strategy as suggested for ExportSpans to ensure all exporters are attempted to be shut down before returning an error.
- if err != nil {
- return fmt.Errorf("could not stop exporter: %w", err)
- }
+ if err != nil {
+ // Collect errors from all exporters
+ allErrors = append(allErrors, err)
+ }
+ }
+ if len(allErrors) > 0 {
+ return fmt.Errorf("could not stop some exporters: %v", allErrors)
+ }Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: codecov/patch
[warning] 46-47: core/metrics/multiexporter.go#L46-L47
Added lines #L46 - L47 were not covered by tests
core/metrics/otlp.go
Outdated
| // create the multi-exporter with optional secondary exporter | ||
| multiExporter := NewMultiExporter(exporter) | ||
| if secondaryClient != nil { | ||
| secondaryExporter, err := otlptrace.New(ctx, secondaryClient) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create secondary otlp exporter: %w", err) | ||
| } | ||
| multiExporter.AddExporter(secondaryExporter) | ||
| } | ||
|
|
||
| n.baseHandler = newBaseHandler( | ||
| n.buildInfo, | ||
| tracesdk.WithBatcher( | ||
| multiExporter, | ||
| tracesdk.WithMaxQueueSize(defaultMaxQueueSize), | ||
| tracesdk.WithMaxExportBatchSize(defaultMaxExportBatch), | ||
| ), | ||
| tracesdk.WithSampler(tracesdk.AlwaysSample()), | ||
| ) |
There was a problem hiding this comment.
Ensure test coverage for multi-exporter logic.
The introduction of the multi-exporter logic is a significant enhancement. However, static analysis indicates that the line where the multi-exporter is created with only the primary exporter (line 67) is not covered by tests.
It's crucial to ensure that both scenarios (with and without the secondary exporter) are covered by unit tests to verify that the multi-exporter behaves as expected in all configurations.
Consider adding unit tests to cover these scenarios to ensure robust error handling and functionality verification.
Tools
GitHub Check: codecov/patch
[warning] 67-67: core/metrics/otlp.go#L67
Added line #L67 was not covered by tests
Tools
GitHub Check: codecov/patch
[warning] 62-63: core/metrics/otlp.go#L62-L63
Added lines #L62 - L63 were not covered by tests
[warning] 65-65: core/metrics/otlp.go#L65
Added line #L65 was not covered by tests
core/metrics/otlp.go
Outdated
| // buildClientFromTransport creates a new OTLP client based on the transport type and url. | ||
| func buildClientFromTransport(transport otlpTransportType, url string) (otlptrace.Client, error) { | ||
| if url == "" { | ||
| return nil, fmt.Errorf("no url specified") | ||
| } | ||
|
|
||
| switch transport { | ||
| case otlpTransportHTTP: | ||
| return otlptracehttp.NewClient(otlptracehttp.WithEndpointURL(url)), nil | ||
| case otlpTransportGRPC: | ||
| return otlptracegrpc.NewClient(otlptracegrpc.WithEndpointURL(url)), nil | ||
| default: | ||
| return nil, fmt.Errorf("unknown transport type: %s", transport.String()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve error handling in buildClientFromTransport.
The function buildClientFromTransport is crucial for creating clients based on the transport type. However, static analysis indicates that the error handling for unknown transport types (lines 156-157) is not covered by tests.
This lack of coverage could lead to untested paths in production, potentially resulting in runtime errors if an unsupported transport type is accidentally used.
Enhance the robustness of this function by adding unit tests to cover these error scenarios. Additionally, consider logging or monitoring these errors to aid in troubleshooting and maintaining the system's stability.
Tools
GitHub Check: codecov/patch
[warning] 156-157: core/metrics/otlp.go#L156-L157
Added lines #L156 - L157 were not covered by tests
Tools
GitHub Check: codecov/patch
[warning] 152-153: core/metrics/otlp.go#L152-L153
Added lines #L152 - L153 were not covered by tests
[warning] 156-157: core/metrics/otlp.go#L156-L157
Added lines #L156 - L157 were not covered by tests
core/metrics/otlp.go
Outdated
| otlpTransportEnvDefault = "OTEL_EXPORTER_OTLP_TRANSPORT" | ||
| otlpTransportEnvPrimary = "OTEL_EXPORTER_OTLP_TRANSPORT_PRIMARY" | ||
| otlpTransportEnvSecondary = "OTEL_EXPORTER_OTLP_TRANSPORT_SECONDARY" |
There was a problem hiding this comment.
Clarify the use of new environment variables in documentation.
The introduction of new constants for environment variables (otlpTransportEnvPrimary and otlpTransportEnvSecondary) is a key part of this feature. However, it's important to ensure that these new variables are well-documented.
As per the previous reviewer's suggestion, add these environment variables to the metrics/README.md to ensure that users are aware of how to configure the primary and secondary transports.
Would you like me to help update the documentation or open a GitHub issue to track this task?
core/metrics/otlp.go
Outdated
| isDefaultConfig := core.GetEnv(otlpTransportEnvDefault, "") != "" | ||
|
|
||
| var client, secondaryClient otlptrace.Client | ||
| if isDefaultConfig { | ||
| client, err = getClient(otlpTransportEnvDefault) | ||
| if err != nil { | ||
| return fmt.Errorf("could not create default client: %w", err) | ||
| } | ||
| } else { | ||
| // instantiate both the primary and secondary clients | ||
| client, err = getClient(otlpTransportEnvPrimary) | ||
| if err != nil { | ||
| return fmt.Errorf("could not create primary client: %w", err) | ||
| } | ||
|
|
||
| secondaryClient, err = getClient(otlpTransportEnvSecondary) | ||
| if err != nil { | ||
| return fmt.Errorf("could not create secondary client: %w", err) | ||
| } |
There was a problem hiding this comment.
Refactor client creation logic to handle secondary transport conditionally.
The current implementation always attempts to create a secondary client, even if the environment variable for the secondary transport is not set. This could lead to unnecessary error handling or exceptions when the secondary transport is not intended to be used.
Consider refactoring the logic to check if the secondary transport environment variable is set before attempting to create the secondary client. This approach aligns with the previous reviewer's suggestion to tolerate cases where the user doesn't define a secondary transport.
Here's a proposed refactor to conditionally handle the secondary client creation:
- secondaryClient, err = getClient(otlpTransportEnvSecondary)
- if err != nil {
- return fmt.Errorf("could not create secondary client: %w", err)
- }
+ if core.GetEnv(otlpTransportEnvSecondary, "") != "" {
+ secondaryClient, err = getClient(otlpTransportEnvSecondary)
+ if err != nil {
+ return fmt.Errorf("could not create secondary client: %w", err)
+ }
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| isDefaultConfig := core.GetEnv(otlpTransportEnvDefault, "") != "" | |
| var client, secondaryClient otlptrace.Client | |
| if isDefaultConfig { | |
| client, err = getClient(otlpTransportEnvDefault) | |
| if err != nil { | |
| return fmt.Errorf("could not create default client: %w", err) | |
| } | |
| } else { | |
| // instantiate both the primary and secondary clients | |
| client, err = getClient(otlpTransportEnvPrimary) | |
| if err != nil { | |
| return fmt.Errorf("could not create primary client: %w", err) | |
| } | |
| secondaryClient, err = getClient(otlpTransportEnvSecondary) | |
| if err != nil { | |
| return fmt.Errorf("could not create secondary client: %w", err) | |
| } | |
| isDefaultConfig := core.GetEnv(otlpTransportEnvDefault, "") != "" | |
| var client, secondaryClient otlptrace.Client | |
| if isDefaultConfig { | |
| client, err = getClient(otlpTransportEnvDefault) | |
| if err != nil { | |
| return fmt.Errorf("could not create default client: %w", err) | |
| } | |
| } else { | |
| // instantiate both the primary and secondary clients | |
| client, err = getClient(otlpTransportEnvPrimary) | |
| if err != nil { | |
| return fmt.Errorf("could not create primary client: %w", err) | |
| } | |
| if core.GetEnv(otlpTransportEnvSecondary, "") != "" { | |
| secondaryClient, err = getClient(otlpTransportEnvSecondary) | |
| if err != nil { | |
| return fmt.Errorf("could not create secondary client: %w", err) | |
| } | |
| } |
Tools
GitHub Check: codecov/patch
[warning] 35-38: core/metrics/otlp.go#L35-L38
Added lines #L35 - L38 were not covered by tests
[warning] 43-44: core/metrics/otlp.go#L43-L44
Added lines #L43 - L44 were not covered by tests
[warning] 48-49: core/metrics/otlp.go#L48-L49
Added lines #L48 - L49 were not covered by tests
core/metrics/multiexporter.go
Outdated
| // MultiExporter is an interface that allows exporting spans to multiple OTLP trace exporters. | ||
| type MultiExporter interface { | ||
| tracesdk.SpanExporter | ||
| AddExporter(exporter *otlptrace.Exporter) |
There was a problem hiding this comment.
should be using SpanExporter instead since it allows any exporter to be used rather than just otlptrace epxorter.
For example we can use tracetest.NewInMemoryExporter for testing (we already do this here)
| ) | ||
|
|
||
| // MultiExporter is an interface that allows exporting spans to multiple OTLP trace exporters. | ||
| type MultiExporter interface { |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- core/go.mod (2 hunks)
- core/metrics/multiexporter_test.go (1 hunks)
- core/metrics/otlp.go (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- core/metrics/multiexporter_test.go
Additional context used
GitHub Check: codecov/patch
core/metrics/otlp.go
[warning] 49-49: core/metrics/otlp.go#L49
Added line #L49 was not covered by tests
[warning] 54-54: core/metrics/otlp.go#L54
Added line #L54 was not covered by tests
[warning] 173-173: core/metrics/otlp.go#L173
Added line #L173 was not covered by tests
[warning] 177-178: core/metrics/otlp.go#L177-L178
Added lines #L177 - L178 were not covered by tests
GitHub Check: Lint (core)
core/metrics/otlp.go
[failure] 53-53:
non-wrapping format verb for fmt.Errorf. Use%wto format errors (errorlint)
[failure] 193-193:
exported: exported function WithURL should have comment or be unexported (revive)
[failure] 202-202:
exported: exported function WithInsecure should have comment or be unexported (revive)
Additional comments not posted (4)
core/metrics/otlp.go (3)
32-70: LGTM! The refactoredStartmethod improves modularity and flexibility.The changes to the
Startmethod introduce support for multiple exporters, allowing for more flexible data exporting configurations. The creation of a primary exporter and the loop to add additional exporters based on environment variables enhance the modularity of the exporter setup.The use of a
multiExporterto combine all created exporters and integrate it into thebaseHandleris a clean approach to handle multiple export paths.Tools
GitHub Check: codecov/patch
[warning] 49-49: core/metrics/otlp.go#L49
Added line #L49 was not covered by tests
[warning] 54-54: core/metrics/otlp.go#L54
Added line #L54 was not covered by testsGitHub Check: Lint (core)
[failure] 53-53:
non-wrapping format verb for fmt.Errorf. Use%wto format errors (errorlint)
136-159: Add test coverage for the newmakeOTLPExporterfunction.The
makeOTLPExporterfunction is a good addition that abstracts the exporter creation logic based on the transport type and URL. It improves code reusability and maintainability.However, static analysis indicates that some lines in this function are not covered by tests:
Tools
GitHub Check: codecov/patch
[warning] 173-173: core/metrics/otlp.go#L173
Added line #L173 was not covered by testsTo ensure the robustness of this critical function, please add unit tests to cover all possible scenarios, including error cases.
162-180: Add test coverage for error handling inbuildClientFromTransport.The
buildClientFromTransportfunction is a nice addition that creates OTLP clients based on the transport type and options. The use of a switch statement to handle different transport types makes the code readable and maintainable.However, static analysis indicates that the error handling for unknown transport types is not covered by tests:
Tools
GitHub Check: codecov/patch
[warning] 177-178: core/metrics/otlp.go#L177-L178
Added lines #L177 - L178 were not covered by testsTo ensure the robustness of this function, please add unit tests to cover the scenario where an unknown transport type is provided. This will help prevent untested paths in production.
Tools
GitHub Check: codecov/patch
[warning] 173-173: core/metrics/otlp.go#L173
Added line #L173 was not covered by tests
[warning] 177-178: core/metrics/otlp.go#L177-L178
Added lines #L177 - L178 were not covered by testscore/go.mod (1)
66-66: LGTM!The changes to the
google.golang.org/grpcdependency are approved. Making the dependency explicit improves clarity in dependency management and indicates a direct use of gRPC functionalities, which may enhance the project's capabilities for remote procedure calls and service communication.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (4)
core/metrics/export_test.go (1)
4-5: LGTM, but add tests.The code changes are approved.
However, consider adding tests for this function to ensure it behaves as expected.
core/metrics/otlp.go (3)
32-70: Refactoring of theStartmethod looks good!The changes to support multiple exporters and the introduction of the multi-exporter improve the flexibility and modularity of the metrics handler. The use of environment variables for configuration is a good practice.
Consider adding more detailed logging statements to aid in debugging and monitoring the creation and integration of exporters.
Tools
GitHub Check: codecov/patch
[warning] 49-49: core/metrics/otlp.go#L49
Added line #L49 was not covered by tests
[warning] 54-54: core/metrics/otlp.go#L54
Added line #L54 was not covered by testsGitHub Check: Lint (core)
[failure] 53-53:
non-wrapping format verb for fmt.Errorf. Use%wto format errors (errorlint)
136-162: The newmakeOTLPExporterfunction looks good!Abstracting the exporter creation logic into a separate function improves code reusability. The use of
getEnvSuffixallows for flexible configuration based on suffixed environment variables, and callingbuildClientFromTransportseparates the client creation logic from the exporter creation.Consider adding more detailed error messages to aid in debugging. For example:
- return nil, fmt.Errorf("could not create exporter: url is empty") + return nil, fmt.Errorf("could not create exporter: url is empty for suffix %q", envSuffix)- return nil, fmt.Errorf("could not create client from transport: %w", err) + return nil, fmt.Errorf("could not create client from transport %s: %w", transport, err)
265-268: The new constantsdefaultMaxQueueSizeanddefaultMaxExportBatchlook good!Defining these constants improves code readability and maintainability by providing clear and centralized configuration options for the exporter. Using them in the
Startmethod ensures consistent default values for the queue size and export batch size.Consider adding comments to describe the purpose and recommended values for these constants. For example:
const ( // defaultMaxQueueSize is the default maximum queue size for the OTLP exporter. // Recommended value: 1000000 defaultMaxQueueSize = 1000000 // defaultMaxExportBatch is the default maximum export batch size for the OTLP exporter. // Recommended value: 2000 defaultMaxExportBatch = 2000 )
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- core/metrics/export_test.go (1 hunks)
- core/metrics/otlp.go (5 hunks)
- core/metrics/otlp_test.go (1 hunks)
Additional context used
GitHub Check: codecov/patch
core/metrics/otlp.go
[warning] 49-49: core/metrics/otlp.go#L49
Added line #L49 was not covered by tests
[warning] 54-54: core/metrics/otlp.go#L54
Added line #L54 was not covered by tests
[warning] 176-176: core/metrics/otlp.go#L176
Added line #L176 was not covered by tests
[warning] 180-181: core/metrics/otlp.go#L180-L181
Added lines #L180 - L181 were not covered by tests
GitHub Check: Lint (core)
core/metrics/otlp.go
[failure] 53-53:
non-wrapping format verb for fmt.Errorf. Use%wto format errors (errorlint)
[failure] 196-196:
exported: exported function WithURL should have comment or be unexported (revive)
[failure] 205-205:
exported: exported function WithInsecure should have comment or be unexported (revive)
[failure] 220-220:
exported: exported function WithHeaders should have comment or be unexported (revive)
[failure] 239-239:
Magic number: 2, in detected (mnd)
[failure] 240-240:
Magic number: 2, in detected (mnd)
Additional comments not posted (1)
core/metrics/otlp_test.go (1)
9-71: LGTM!The test function
TestHeadersToMapis well-structured and follows the table-driven testing approach. It covers various scenarios and correctly compares the result with the expected output map usingreflect.DeepEqual. The test cases are comprehensive and provide meaningful error messages when the result does not match the expected output.The code changes are approved.
| // buildClientFromTransport creates a new OTLP client based on the transport type and url. | ||
| func buildClientFromTransport(transport otlpTransportType, options ...Option) (otlptrace.Client, error) { | ||
| to := transportOptions{} | ||
| for _, option := range options { | ||
| if err := option(&to); err != nil { | ||
| return nil, fmt.Errorf("could not apply option: %w", err) | ||
| } | ||
| } | ||
|
|
||
| // TODO: make sure url is validated | ||
|
|
||
| switch transport { | ||
| case otlpTransportHTTP: | ||
| return otlptracehttp.NewClient(to.httpOptions...), nil | ||
| case otlpTransportGRPC: | ||
| return otlptracegrpc.NewClient(to.grpcOptions...), nil | ||
| default: | ||
| return nil, fmt.Errorf("unknown transport type: %s", transport.String()) | ||
| } | ||
| } |
There was a problem hiding this comment.
The new buildClientFromTransport function looks good!
Separating the client creation logic based on the transport type makes the code modular and extensible. The use of a switch statement makes it easy to add support for new transport types in the future, and applying the provided options allows for flexible configuration of the transport client.
However, static analysis tools have reported a lack of test coverage for certain lines in this function.
Please add unit tests to cover the following scenarios:
- Creating an HTTP transport client (line 176)
- Creating a gRPC transport client (line 178)
- Handling an unknown transport type (lines 180-181)
This will ensure that all code paths in the function are properly tested.
Tools
GitHub Check: codecov/patch
[warning] 176-176: core/metrics/otlp.go#L176
Added line #L176 was not covered by tests
[warning] 180-181: core/metrics/otlp.go#L180-L181
Added lines #L180 - L181 were not covered by tests
core/metrics/otlp.go
Outdated
| func WithURL(url string) Option { | ||
| return func(o *transportOptions) error { | ||
| o.httpOptions = append(o.httpOptions, otlptracehttp.WithEndpointURL(url)) | ||
| o.grpcOptions = append(o.grpcOptions, otlptracegrpc.WithEndpointURL(url)) | ||
|
|
||
| return nil | ||
| } | ||
| } | ||
|
|
||
| func WithInsecure(isInsecure bool) Option { | ||
| return func(o *transportOptions) error { | ||
| if isInsecure { | ||
| o.httpOptions = append(o.httpOptions, otlptracehttp.WithInsecure()) | ||
| o.grpcOptions = append(o.grpcOptions, otlptracegrpc.WithInsecure()) | ||
| } else { | ||
| tlsCreds := credentials.NewClientTLSFromCert(nil, "") | ||
| // note: you do not need to specify the tls creds for http, this happens automatically when https:// is used as the protocol scheme. | ||
| o.grpcOptions = append(o.grpcOptions, otlptracegrpc.WithTLSCredentials(tlsCreds)) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
| } | ||
|
|
||
| func WithHeaders(headers string) Option { | ||
| return func(o *transportOptions) error { | ||
| realHeaders := headersToMap(headers) | ||
| o.httpOptions = append(o.httpOptions, otlptracehttp.WithHeaders(realHeaders)) | ||
| o.grpcOptions = append(o.grpcOptions, otlptracegrpc.WithHeaders(realHeaders)) | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
The new option functions WithURL, WithInsecure, and WithHeaders look good!
Providing these option functions allows for clean and extensible configuration of the OTLP exporter. Exporting them enables flexibility in configuring the exporter from other packages.
However, static analysis tools have reported that these functions should have comments or be unexported.
Please add comments for the exported functions to improve code documentation and maintainability. For example:
// WithURL sets the endpoint URL for the OTLP exporter.
func WithURL(url string) Option {
// ...
}
// WithInsecure enables or disables secure connection for the OTLP exporter.
func WithInsecure(isInsecure bool) Option {
// ...
}
// WithHeaders sets the headers for the OTLP exporter.
func WithHeaders(headers string) Option {
// ...
}Tools
GitHub Check: Lint (core)
[failure] 196-196:
exported: exported function WithURL should have comment or be unexported (revive)
[failure] 205-205:
exported: exported function WithInsecure should have comment or be unexported (revive)
[failure] 220-220:
exported: exported function WithHeaders should have comment or be unexported (revive)
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
core/metrics/otlp.go (1)
197-228: The new option functions look good, but add comments for exported functions.The new option functions
WithURL,WithInsecure, andWithHeadersprovide a clean and extensible way to configure the OTLP exporter. They allow for flexible configuration of the exporter by appending the corresponding options for both HTTP and gRPC transports.However, static analysis tools have reported that these exported functions should have comments to improve code documentation and maintainability.
Please add comments for the exported functions. For example:
// WithURL sets the endpoint URL for the OTLP exporter. func WithURL(url string) Option { // ... } // WithInsecure enables or disables secure connection for the OTLP exporter. func WithInsecure(isInsecure bool) Option { // ... } // WithHeaders sets the headers for the OTLP exporter. func WithHeaders(headers string) Option { // ... }Tools
GitHub Check: Lint (core)
[failure] 197-197:
exported: exported function WithURL should have comment or be unexported (revive)
[failure] 206-206:
exported: exported function WithInsecure should have comment or be unexported (revive)
[failure] 221-221:
exported: exported function WithHeaders should have comment or be unexported (revive)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- core/metrics/otlp.go (5 hunks)
Additional context used
GitHub Check: codecov/patch
core/metrics/otlp.go
[warning] 49-49: core/metrics/otlp.go#L49
Added line #L49 was not covered by tests
[warning] 54-54: core/metrics/otlp.go#L54
Added line #L54 was not covered by tests
[warning] 177-177: core/metrics/otlp.go#L177
Added line #L177 was not covered by tests
[warning] 181-182: core/metrics/otlp.go#L181-L182
Added lines #L181 - L182 were not covered by tests
GitHub Check: Lint (core)
core/metrics/otlp.go
[failure] 53-53:
non-wrapping format verb for fmt.Errorf. Use%wto format errors (errorlint)
[failure] 197-197:
exported: exported function WithURL should have comment or be unexported (revive)
[failure] 206-206:
exported: exported function WithInsecure should have comment or be unexported (revive)
[failure] 221-221:
exported: exported function WithHeaders should have comment or be unexported (revive)
Additional comments not posted (4)
core/metrics/otlp.go (4)
32-70: Refactoring of theStartmethod looks good!The changes to the
Startmethod significantly improve the modularity and flexibility of the exporter setup. The creation of a primary exporter and the loop to add additional exporters based on environment variables enhances the configurability of the metrics handler. The use of amultiExportersimplifies the integration of multiple exporters into thebaseHandler.The addition of constants for maximum queue size and export batch size provides clearer configuration options for the exporter behavior.
Tools
GitHub Check: codecov/patch
[warning] 49-49: core/metrics/otlp.go#L49
Added line #L49 was not covered by tests
[warning] 54-54: core/metrics/otlp.go#L54
Added line #L54 was not covered by testsGitHub Check: Lint (core)
[failure] 53-53:
non-wrapping format verb for fmt.Errorf. Use%wto format errors (errorlint)
137-163: The newmakeOTLPExporterfunction looks good!The
makeOTLPExporterfunction encapsulates the logic for creating an OTLP exporter based on the transport type and endpoint. It abstracts the details of creating the OTLP client by using thebuildClientFromTransportfunction. The function handles errors appropriately and returns the created exporter.
165-184: The newbuildClientFromTransportfunction looks good, but add more unit tests.The
buildClientFromTransportfunction provides a clean and extensible way to create OTLP clients based on the transport type and options. It applies the provided options to the transport-specific options, allowing for flexible configuration. The use of a switch statement makes it easy to add support for new transport types in the future.However, static analysis tools have reported a lack of test coverage for certain lines in this function.
Please add unit tests to cover the following scenarios:
- Creating an HTTP transport client (line 177)
- Creating a gRPC transport client (line 179)
- Handling an unknown transport type (lines 181-182)
This will ensure that all code paths in the function are properly tested.
Tools
GitHub Check: codecov/patch
[warning] 177-177: core/metrics/otlp.go#L177
Added line #L177 was not covered by tests
[warning] 181-182: core/metrics/otlp.go#L181-L182
Added lines #L181 - L182 were not covered by tests
49-49: Improve test coverage for theotlpHandlerandbuildClientFromTransport.Static analysis tools have reported a lack of test coverage for the following lines:
- Line 49: Breaking out of the loop when no more transports are available.
- Line 54: Error handling when creating an exporter fails.
- Line 177: Creating an HTTP transport client.
- Lines 181-182: Handling an unknown transport type.
Please add unit tests to cover these scenarios and improve the overall test coverage of the file. This will ensure that all code paths are properly tested and increase confidence in the correctness of the implementation.
Also applies to: 54-54, 177-177, 181-182
Tools
GitHub Check: codecov/patch
[warning] 49-49: core/metrics/otlp.go#L49
Added line #L49 was not covered by tests
core/metrics/otlp.go
Outdated
|
|
||
| exporter, err := makeOTLPExporter(ctx, envSuffix) | ||
| if err != nil { | ||
| return fmt.Errorf("could not create exporter %d: %v", i, err) |
There was a problem hiding this comment.
Address the non-wrapping format verb for fmt.Errorf.
Static analysis tools have reported the following issue:
non-wrapping format verb for fmt.Errorf. Use
%wto format errors (errorlint)
Please use %w to format errors for better error handling and wrapping. Apply this diff to fix the issue:
-return fmt.Errorf("could not create exporter %d: %v", i, err)
+return fmt.Errorf("could not create exporter %d: %w", i, err)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return fmt.Errorf("could not create exporter %d: %v", i, err) | |
| return fmt.Errorf("could not create exporter %d: %w", i, err) |
Tools
GitHub Check: Lint (core)
[failure] 53-53:
non-wrapping format verb for fmt.Errorf. Use%wto format errors (errorlint)
trajan0x
force-pushed
the
multiple-exports
branch
from
387abc9 to
732f5fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- core/metrics/README.md (1 hunks)
- core/metrics/otlp.go (5 hunks)
Additional context used
GitHub Check: codecov/patch
core/metrics/otlp.go
[warning] 50-50: core/metrics/otlp.go#L50
Added line #L50 was not covered by tests
[warning] 55-55: core/metrics/otlp.go#L55
Added line #L55 was not covered by tests
[warning] 58-58: core/metrics/otlp.go#L58
Added line #L58 was not covered by tests
[warning] 194-194: core/metrics/otlp.go#L194
Added line #L194 was not covered by tests
[warning] 198-199: core/metrics/otlp.go#L198-L199
Added lines #L198 - L199 were not covered by tests
GitHub Check: Lint (core)
core/metrics/otlp.go
[failure] 54-54:
non-wrapping format verb for fmt.Errorf. Use%wto format errors (errorlint)
[failure] 230-230:
G402: TLS MinVersion too low. (gosec)
[failure] 214-214:
exported: exported function WithURL should have comment or be unexported (revive)
[failure] 223-223:
exported: exported function WithSecure should have comment or be unexported (revive)
[failure] 243-243:
exported: exported function WithHeaders should have comment or be unexported (revive)
Additional comments not posted (6)
core/metrics/README.md (2)
17-30: LGTM!The table provides clear and useful information about the additional environment variables for configuring multiple OTLP exporters. The default values are appropriate placeholders.
33-33: LGTM!The note provides clear and accurate information about configuring multiple OTLP exporters using incrementing numbers in the environment variable names. It aligns well with the details provided in the table above.
core/metrics/otlp.go (4)
33-71: Refactoring of theStartmethod looks good!The changes to the
Startmethod introduce support for multiple exporters, which improves the flexibility and modularity of the exporter setup. The creation of a primary exporter and the loop to add additional exporters based on environment variables is a clean and extensible approach.The introduction of the
multiExporterto combine all created exporters and integrate it into thebaseHandleris a nice way to manage multiple export paths.Overall, these changes enhance the robustness and configurability of the metrics handling.
Tools
GitHub Check: codecov/patch
[warning] 50-50: core/metrics/otlp.go#L50
Added line #L50 was not covered by tests
[warning] 55-55: core/metrics/otlp.go#L55
Added line #L55 was not covered by tests
[warning] 58-58: core/metrics/otlp.go#L58
Added line #L58 was not covered by testsGitHub Check: Lint (core)
[failure] 54-54:
non-wrapping format verb for fmt.Errorf. Use%wto format errors (errorlint)
289-290: New constants for configuration look good!The addition of the
defaultMaxQueueSizeanddefaultMaxExportBatchconstants is a good practice for maintainability and readability. Using these constants in theStartmethod when creating thebaseHandlerprovides clearer configuration options for the exporter behavior.
141-180: NewmakeOTLPExporterfunction looks good!The introduction of the
makeOTLPExporterfunction is a good abstraction for creating OTLP exporters. It encapsulates the logic for creating exporters based on the transport type and URL, making the code more modular and reusable.The use of the
envSuffixparameter to support creating exporters with different configurations is a nice touch, as it allows for more flexible setup.The function also uses the
buildClientFromTransportfunction to create the OTLP client, which further improves the modularity of the code.Overall, this function is a good addition to the codebase.
182-201: The newbuildClientFromTransportfunction looks good!Separating the client creation logic based on the transport type makes the code modular and extensible. The use of a switch statement makes it easy to add support for new transport types in the future, and applying the provided options allows for flexible configuration of the transport client.
However, static analysis tools have reported a lack of test coverage for certain lines in this function.
Please add unit tests to cover the following scenarios:
- Creating an HTTP transport client (line 195)
- Creating a gRPC transport client (line 197)
- Handling an unknown transport type (lines 198-199)
This will ensure that all code paths in the function are properly tested.
Tools
GitHub Check: codecov/patch
[warning] 194-194: core/metrics/otlp.go#L194
Added line #L194 was not covered by tests
[warning] 198-199: core/metrics/otlp.go#L198-L199
Added lines #L198 - L199 were not covered by tests
core/metrics/README.md
Outdated
| <!-- TODO: fully document before pr merged--> | ||
|
|
There was a problem hiding this comment.
Address the TODO comment.
Please ensure that the documentation is fully updated before merging the PR, as indicated by the TODO comment.
If you need any assistance with completing the documentation, feel free to let me know.
core/metrics/otlp.go
Outdated
| func WithURL(url string) Option { | ||
| return func(o *transportOptions) error { | ||
| o.httpOptions = append(o.httpOptions, otlptracehttp.WithEndpointURL(url)) | ||
| o.grpcOptions = append(o.grpcOptions, otlptracegrpc.WithEndpointURL(url)) | ||
|
|
||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
The new option functions WithURL, WithInsecure, and WithHeaders look good!
Providing these option functions allows for clean and extensible configuration of the OTLP exporter. Exporting them enables flexibility in configuring the exporter from other packages.
The functions modify the transportOptions struct in a consistent way, appending the corresponding options for both HTTP and gRPC transports, which is a good practice for maintaining compatibility between the two transport types.
However, static analysis tools have reported that these functions should have comments or be unexported.
Please add comments for the exported functions to improve code documentation and maintainability. For example:
// WithURL sets the endpoint URL for the OTLP exporter.
func WithURL(url string) Option {
// ...
}
// WithInsecure enables or disables secure connection for the OTLP exporter.
func WithInsecure(isInsecure bool) Option {
// ...
}
// WithHeaders sets the headers for the OTLP exporter.
func WithHeaders(headers string) Option {
// ...
}Also applies to: 223-241, 243-250
Tools
GitHub Check: Lint (core)
[failure] 214-214:
exported: exported function WithURL should have comment or be unexported (revive)
core/metrics/otlp.go
Outdated
| func WithSecure(secure bool) Option { | ||
| return func(o *transportOptions) error { | ||
| if secure { | ||
| tlsCreds := credentials.NewClientTLSFromCert(nil, "") | ||
| // note: you do not need to specify the tls creds for http, this happens automatically when https:// is used as the protocol scheme. | ||
| o.grpcOptions = append(o.grpcOptions, otlptracegrpc.WithTLSCredentials(tlsCreds)) | ||
|
|
||
| tlsConfig := &tls.Config{ | ||
| // RootCAs is nil, which means the default system root CAs are used | ||
| } | ||
| o.httpOptions = append(o.httpOptions, otlptracehttp.WithTLSClientConfig(tlsConfig)) | ||
| } else { | ||
| o.httpOptions = append(o.httpOptions, otlptracehttp.WithInsecure()) | ||
| o.grpcOptions = append(o.grpcOptions, otlptracegrpc.WithInsecure()) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve TLS configuration in the WithSecure function.
The WithSecure function is an important option for configuring secure connections for the OTLP exporter. It correctly sets the TLS credentials for gRPC transport and the TLS configuration for HTTP transport based on the secure parameter.
However, static analysis tools have reported that the TLS MinVersion is too low.
To ensure better security, consider setting a higher MinVersion for the TLS configuration. For example:
tlsConfig := &tls.Config{
MinVersion: tls.VersionTLS12,
// ...
}Using tls.VersionTLS12 or higher is recommended for secure connections.
Tools
GitHub Check: Lint (core)
[failure] 230-230:
G402: TLS MinVersion too low. (gosec)
[failure] 223-223:
exported: exported function WithSecure should have comment or be unexported (revive)
core/metrics/otlp.go
Outdated
| primaryExporter, err := makeOTLPExporter(ctx, "") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create otlp exporter: %w", err) | ||
| return fmt.Errorf("could not create default client: %w", err) | ||
| } | ||
| exporters = append(exporters, primaryExporter) | ||
|
|
||
| // Loop to create additional exporters | ||
| for i := 1; ; i++ { | ||
| envSuffix := fmt.Sprintf("_%d", i) | ||
| // if this is empty we can assume no config exists at all. | ||
| endpointEnv := otelEndpointEnv + envSuffix | ||
|
|
||
| // no more transports to add. | ||
| if !core.HasEnv(endpointEnv) { | ||
| break | ||
| } | ||
|
|
||
| n.baseHandler = newBaseHandler(n.buildInfo, tracesdk.WithBatcher(exporter, tracesdk.WithMaxQueueSize(1000000), tracesdk.WithMaxExportBatchSize(2000)), tracesdk.WithSampler(tracesdk.AlwaysSample())) | ||
| exporter, err := makeOTLPExporter(ctx, envSuffix) | ||
| if err != nil { | ||
| return fmt.Errorf("could not create exporter %d: %v", i, err) | ||
| } | ||
|
|
||
| exporters = append(exporters, exporter) | ||
| } |
There was a problem hiding this comment.
Improve error handling in the Start method and the buildClientFromTransport function.
The error handling in the Start method and the buildClientFromTransport function is generally good. The Start method correctly returns an error if the creation of any exporter fails, which is important for error propagation. The buildClientFromTransport function also handles the case of an unknown transport type by returning an error, which is crucial for catching invalid configurations.
However, static analysis tools have reported a non-wrapping format verb for fmt.Errorf in the Start method.
To improve error wrapping and preserve the original error context, use the %w verb for fmt.Errorf. Apply this diff to fix the issue:
-return fmt.Errorf("could not create exporter %d: %v", i, err)
+return fmt.Errorf("could not create exporter %d: %w", i, err)Using %w ensures that the original error is wrapped properly and can be unwrapped later if needed.
Also applies to: 198-199
Tools
GitHub Check: codecov/patch
[warning] 50-50: core/metrics/otlp.go#L50
Added line #L50 was not covered by tests
[warning] 55-55: core/metrics/otlp.go#L55
Added line #L55 was not covered by tests
[warning] 58-58: core/metrics/otlp.go#L58
Added line #L58 was not covered by tests
GitHub Check: Lint (core)
[failure] 54-54:
non-wrapping format verb for fmt.Errorf. Use%wto format errors (errorlint)
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range and nitpick comments (3)
core/metrics/multiexporter.go (1)
41-69: LGTM, but consider making the timeout configurable.The
doParallelfunction is well-implemented, ensuring proper concurrency safety and error handling. The use ofWaitGroupandmutexis appropriate for managing the goroutines and collecting errors.The timeout of 30 seconds is a reasonable default, but it might be worth considering making it configurable to allow users to adjust it based on their specific requirements.
To make the timeout configurable, you can add a
timeoutfield to themultiExporterstruct and use it in thedoParallelfunction:type multiExporter struct { exporters []trace.SpanExporter + timeout time.Duration } -func NewMultiExporter(exporters ...trace.SpanExporter) MultiExporter { +func NewMultiExporter(exporters []trace.SpanExporter, timeout time.Duration) MultiExporter { return &multiExporter{ exporters: exporters, + timeout: timeout, } } func (m *multiExporter) doParallel(parentCtx context.Context, fn func(context.Context, trace.SpanExporter) error) error { - ctx, cancel := context.WithTimeout(parentCtx, defaultTimeout) + ctx, cancel := context.WithTimeout(parentCtx, m.timeout) defer cancel() // ... }Tools
GitHub Check: codecov/patch
[warning] 50-50: core/metrics/multiexporter.go#L50
Added line #L50 was not covered by tests
[warning] 68-68: core/metrics/multiexporter.go#L68
Added line #L68 was not covered by testscore/metrics/README.md (1)
15-16: Enhance the explanation of the multi-exporter functionality.The section explaining the multi-exporter functionality could benefit from a more detailed explanation or example. The current text mentions that the
multiexporter.gofile "simply wraps multiple otel clients," but it does not explain how this is achieved or how users can configure or utilize this functionality effectively.Consider adding a code snippet or a more detailed step-by-step guide on setting up multiple exporters using the provided environment variables.
core/metrics/otlp.go (1)
214-214: Add comments for the exported option functions.Static analysis tools have reported that the exported functions
withURL,withSecure, andwithHeadersshould have comments or be unexported.To improve code documentation and maintainability, add comments for these functions. For example:
// withURL sets the endpoint URL for the OTLP exporter. func withURL(url string) Option { // ... } // withSecure enables or disables secure connection for the OTLP exporter. func withSecure(secure bool) Option { // ... } // withHeaders sets the headers for the OTLP exporter. func withHeaders(headers string) Option { // ... }Also applies to: 223-223, 244-244
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- core/metrics/README.md (1 hunks)
- core/metrics/multiexporter.go (1 hunks)
- core/metrics/otlp.go (5 hunks)
Additional context used
GitHub Check: codecov/patch
core/metrics/multiexporter.go
[warning] 50-50: core/metrics/multiexporter.go#L50
Added line #L50 was not covered by tests
[warning] 68-68: core/metrics/multiexporter.go#L68
Added line #L68 was not covered by testscore/metrics/otlp.go
[warning] 50-50: core/metrics/otlp.go#L50
Added line #L50 was not covered by tests
[warning] 55-55: core/metrics/otlp.go#L55
Added line #L55 was not covered by tests
[warning] 58-58: core/metrics/otlp.go#L58
Added line #L58 was not covered by tests
[warning] 194-194: core/metrics/otlp.go#L194
Added line #L194 was not covered by tests
[warning] 198-199: core/metrics/otlp.go#L198-L199
Added lines #L198 - L199 were not covered by tests
Additional comments not posted (9)
core/metrics/multiexporter.go (3)
14-17: LGTM!The
MultiExporterinterface is well-defined, extending thetrace.SpanExporterinterface and adding anAddExportermethod to support multiple exporters.
26-30: LGTM!The
NewMultiExporterfunction correctly initializes a newmultiExporterinstance with the providedtrace.SpanExporterinstances.
79-81: LGTM!The
AddExportermethod correctly adds the providedtrace.SpanExporterto the list of registered exporters.core/metrics/README.md (1)
17-30: LGTM!The table clearly documents the additional environment variables for configuring multiple OTLP exporters. The naming convention and default values are well-defined.
core/metrics/otlp.go (5)
33-71: Refactoring of theStartmethod to support multiple exporters looks good!The changes in the
Startmethod to create a primary exporter and additional exporters based on environment variables are well-structured and modular. The usage of themakeOTLPExporterfunction abstracts the exporter creation logic, making the code more readable.The creation of the multi-exporter and its integration into the
baseHandleris a good approach to combine multiple exporters.Tools
GitHub Check: codecov/patch
[warning] 50-50: core/metrics/otlp.go#L50
Added line #L50 was not covered by tests
[warning] 55-55: core/metrics/otlp.go#L55
Added line #L55 was not covered by tests
[warning] 58-58: core/metrics/otlp.go#L58
Added line #L58 was not covered by tests
140-180: The newmakeOTLPExporterfunction is a good abstraction for creating OTLP exporters.The
makeOTLPExporterfunction encapsulates the logic for creating an OTLP exporter based on the transport type and URL, making the code more modular and reusable. It properly handles error scenarios and returns an error if the exporter creation fails, which is important for error propagation and handling in the calling code.The usage of the
buildClientFromTransportfunction to create the OTLP client is a good approach to separate the client creation logic based on the transport type.
182-201: The newbuildClientFromTransportfunction is a good abstraction for creating OTLP clients.The
buildClientFromTransportfunction provides a clean and extensible way to create OTLP clients based on the transport type. It applies the provided options to configure the transport client, making it flexible and reusable.The function correctly handles the case of an unknown transport type by returning an error, which is crucial for catching invalid configurations.
Tools
GitHub Check: codecov/patch
[warning] 194-194: core/metrics/otlp.go#L194
Added line #L194 was not covered by tests
[warning] 198-199: core/metrics/otlp.go#L198-L199
Added lines #L198 - L199 were not covered by tests
289-292: The new constants for maximum queue size and export batch size are a good addition.The introduction of the
defaultMaxQueueSizeanddefaultMaxExportBatchconstants provides clear configuration options for the exporter behavior. It makes the code more readable and maintainable by giving meaningful names to the values used for queue size and batch size.The constants are properly used in the
Startmethod when creating thebaseHandler, ensuring that the exporter is configured with the desired queue size and batch size.
214-221: The new option functionswithURL,withSecure, andwithHeadersare a good addition for configuring the OTLP exporter.Providing these option functions allows for clean and extensible configuration of the OTLP exporter. Exporting them enables flexibility in configuring the exporter from other packages.
The functions modify the
transportOptionsstruct in a consistent way, appending the corresponding options for both HTTP and gRPC transports, which is a good practice for maintaining compatibility between the two transport types.Also applies to: 223-242, 244-251
| func (m *multiExporter) doParallel(parentCtx context.Context, fn func(context.Context, trace.SpanExporter) error) error { | ||
| ctx, cancel := context.WithTimeout(parentCtx, defaultTimeout) | ||
| defer cancel() | ||
|
|
||
| var wg sync.WaitGroup | ||
| var errors []error | ||
| var mu sync.Mutex | ||
|
|
||
| wg.Add(len(m.exporters)) | ||
| for _, exporter := range m.exporters { | ||
| go func(exporter trace.SpanExporter) { | ||
| defer wg.Done() | ||
| err := fn(ctx, exporter) | ||
| if err != nil { | ||
| mu.Lock() | ||
| errors = append(errors, fmt.Errorf("error in doMultiple: %w", err)) | ||
| mu.Unlock() | ||
| } | ||
| }(exporter) | ||
| } | ||
|
|
||
| wg.Wait() | ||
| if len(errors) > 0 { | ||
| // nolint: wrapcheck | ||
| return multierr.Combine(errors...) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Ensure test coverage for the doParallel function.
The static analysis hints indicate that some lines in the doParallel function (lines 50 and 68) are not covered by tests.
Given that doParallel is a critical part of the multiExporter implementation, it is important to ensure that it is thoroughly tested, including the case when no errors occur.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 50-50: core/metrics/multiexporter.go#L50
Added line #L50 was not covered by tests
[warning] 68-68: core/metrics/multiexporter.go#L68
Added line #L68 was not covered by tests
| func (m *multiExporter) ExportSpans(parentCtx context.Context, ss []trace.ReadOnlySpan) error { | ||
| return m.doParallel(parentCtx, func(ctx context.Context, exporter trace.SpanExporter) error { | ||
| return exporter.ExportSpans(ctx, ss) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Consider collecting errors from all exporters before returning.
The current implementation returns immediately if an error occurs while exporting to one of the exporters. This means that if one exporter fails, the remaining exporters will not receive the spans.
It would be better to collect errors from all exporters and return a combined error. This way, all exporters will have a chance to receive the spans, and the caller will be informed of all the errors that occurred.
Apply this diff to collect errors from all exporters:
func (m *multiExporter) ExportSpans(parentCtx context.Context, ss []trace.ReadOnlySpan) error {
- return m.doParallel(parentCtx, func(ctx context.Context, exporter trace.SpanExporter) error {
- return exporter.ExportSpans(ctx, ss)
- })
+ var errs []error
+ m.doParallel(parentCtx, func(ctx context.Context, exporter trace.SpanExporter) error {
+ err := exporter.ExportSpans(ctx, ss)
+ if err != nil {
+ errs = append(errs, err)
+ }
+ return nil
+ })
+ if len(errs) > 0 {
+ return fmt.Errorf("could not export spans to some exporters: %v", errs)
+ }
+ return nil
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m *multiExporter) ExportSpans(parentCtx context.Context, ss []trace.ReadOnlySpan) error { | |
| return m.doParallel(parentCtx, func(ctx context.Context, exporter trace.SpanExporter) error { | |
| return exporter.ExportSpans(ctx, ss) | |
| }) | |
| } | |
| func (m *multiExporter) ExportSpans(parentCtx context.Context, ss []trace.ReadOnlySpan) error { | |
| var errs []error | |
| m.doParallel(parentCtx, func(ctx context.Context, exporter trace.SpanExporter) error { | |
| err := exporter.ExportSpans(ctx, ss) | |
| if err != nil { | |
| errs = append(errs, err) | |
| } | |
| return nil | |
| }) | |
| if len(errs) > 0 { | |
| return fmt.Errorf("could not export spans to some exporters: %v", errs) | |
| } | |
| return nil | |
| } |
| func (m *multiExporter) Shutdown(ctx context.Context) error { | ||
| return m.doParallel(ctx, func(ctx context.Context, exporter trace.SpanExporter) error { | ||
| return exporter.Shutdown(ctx) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Consider collecting errors from all exporters before returning.
Similar to the ExportSpans method, the current implementation of Shutdown returns immediately if an error occurs while shutting down one of the exporters. This means that if one exporter fails to shut down, the remaining exporters will not be shut down.
As suggested earlier, it would be better to collect errors from all exporters and return a combined error. This way, all exporters will be attempted to be shut down, and the caller will be informed of all the errors that occurred.
Apply this diff to collect errors from all exporters:
func (m *multiExporter) Shutdown(ctx context.Context) error {
- return m.doParallel(ctx, func(ctx context.Context, exporter trace.SpanExporter) error {
- return exporter.Shutdown(ctx)
- })
+ var errs []error
+ m.doParallel(ctx, func(ctx context.Context, exporter trace.SpanExporter) error {
+ err := exporter.Shutdown(ctx)
+ if err != nil {
+ errs = append(errs, err)
+ }
+ return nil
+ })
+ if len(errs) > 0 {
+ return fmt.Errorf("could not stop some exporters: %v", errs)
+ }
+ return nil
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m *multiExporter) Shutdown(ctx context.Context) error { | |
| return m.doParallel(ctx, func(ctx context.Context, exporter trace.SpanExporter) error { | |
| return exporter.Shutdown(ctx) | |
| }) | |
| } | |
| func (m *multiExporter) Shutdown(ctx context.Context) error { | |
| var errs []error | |
| m.doParallel(ctx, func(ctx context.Context, exporter trace.SpanExporter) error { | |
| err := exporter.Shutdown(ctx) | |
| if err != nil { | |
| errs = append(errs, err) | |
| } | |
| return nil | |
| }) | |
| if len(errs) > 0 { | |
| return fmt.Errorf("could not stop some exporters: %v", errs) | |
| } | |
| return nil | |
| } |
|
|
||
| You can do the same thing for `OTEL_EXPORTER_OTLP_SECURE_MODE` and `OTEL_EXPORTER_OTLP_HEADERS` | ||
|
|
||
| <!-- TODO: fully document these optins--> |
There was a problem hiding this comment.
Address the TODO comment.
Please ensure that the documentation is fully updated before merging the PR, as indicated by the TODO comment.
If you need any assistance with completing the documentation, feel free to let me know.
| // note: you do not need to specify the tls creds for http, this happens automatically when https:// is used as the protocol scheme. | ||
| o.grpcOptions = append(o.grpcOptions, otlptracegrpc.WithTLSCredentials(tlsCreds)) | ||
|
|
||
| tlsConfig := &tls.Config{ |
There was a problem hiding this comment.
Improve TLS configuration in the withSecure function.
The withSecure function is an important option for configuring secure connections for the OTLP exporter. It correctly sets the TLS credentials for gRPC transport and the TLS configuration for HTTP transport based on the secure parameter.
However, static analysis tools have reported that the TLS MinVersion is too low.
To ensure better security, consider setting a higher MinVersion for the TLS configuration. For example:
tlsConfig := &tls.Config{
MinVersion: tls.VersionTLS12,
// ...
}Using tls.VersionTLS12 or higher is recommended for secure connections.
| n.baseHandler = newBaseHandler(n.buildInfo, tracesdk.WithBatcher(exporter, tracesdk.WithMaxQueueSize(1000000), tracesdk.WithMaxExportBatchSize(2000)), tracesdk.WithSampler(tracesdk.AlwaysSample())) | ||
| exporter, err := makeOTLPExporter(ctx, envSuffix) | ||
| if err != nil { | ||
| return fmt.Errorf("could not create exporter %d: %w", i, err) |
There was a problem hiding this comment.
Improve error wrapping in the Start method.
The error handling in the Start method is generally good. It correctly returns an error if the creation of any exporter fails, which is important for error propagation.
However, static analysis tools have reported a non-wrapping format verb for fmt.Errorf in the Start method.
To improve error wrapping and preserve the original error context, use the %w verb for fmt.Errorf. Apply this diff to fix the issue:
-return fmt.Errorf("could not create exporter %d: %v", i, err)
+return fmt.Errorf("could not create exporter %d: %w", i, err)Using %w ensures that the original error is wrapped properly and can be unwrapped later if needed.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return fmt.Errorf("could not create exporter %d: %w", i, err) | |
| return fmt.Errorf("could not create exporter %d: %w", i, err) |
| // no more transports to add. | ||
| if !core.HasEnv(endpointEnv) { | ||
| break | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Add Tests for Error Handling in Multi-Exporter Logic
The current test TestMultiExporter verifies the successful operation of the multi-exporter logic but does not cover error handling scenarios such as exporter creation failure or breaking out of the loop when no exporters are configured. It is recommended to add unit tests to cover these scenarios to ensure robustness.
- Consider adding tests for:
- Breaking out of the loop when no more exporters are configured.
- Error handling when exporter creation fails.
Analysis chain
Ensure test coverage for the multi-exporter logic and error handling scenarios.
Static analysis tools have reported that some lines in the Start method are not covered by tests:
- Line 50: Breaking out of the loop when no more exporters are configured
- Line 55: Error handling when exporter creation fails
- Line 58: Appending the created exporter to the list of exporters
It's important to add unit tests to cover these scenarios and ensure the robustness of the multi-exporter logic and error handling.
Also applies to: 55-55, 58-58
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for the multi-exporter logic and error handling scenarios.
# Test: Search for tests covering the multi-exporter logic. Expect: At least one test.
rg --type go $'func Test.*MultiExporter'
# Test: Search for tests covering the error handling scenarios. Expect: At least one test.
rg --type go $'func Test.*ExporterCreationError'
Length of output: 165
Script:
#!/bin/bash
# Search for the implementation of the TestMultiExporter function to check its coverage.
rg --type go -A 20 'func TestMultiExporter' core/metrics/multiexporter_test.go
# Search for other test functions in the same file to see if they cover error handling.
rg --type go 'func Test' core/metrics/multiexporter_test.go
Length of output: 825
Tools
GitHub Check: codecov/patch
[warning] 50-50: core/metrics/otlp.go#L50
Added line #L50 was not covered by tests
| default: | ||
| return nil, fmt.Errorf("unknown transport type: %s", transport.String()) |
There was a problem hiding this comment.
Tip
Codebase Verification
Add unit tests for error handling in buildClientFromTransport.
The error handling for unknown transport types in the buildClientFromTransport function is not currently covered by any tests. It is important to add unit tests to cover this scenario and ensure the robustness of the function.
- Consider adding a test case that provides an invalid transport type and verifies that an error is returned.
Analysis chain
Improve error handling in the buildClientFromTransport function.
Static analysis tools have reported that the error handling for unknown transport types (lines 198-199) is not covered by tests.
It's important to add unit tests to cover this scenario and ensure the robustness of the function. Consider adding a test case that provides an invalid transport type and verifies that an error is returned.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for the error handling of unknown transport types.
# Test: Search for tests covering the error handling of unknown transport types. Expect: At least one test.
rg --type go $'func Test.*UnknownTransportType'
Length of output: 48
Script:
#!/bin/bash
# Description: Search for test cases involving the buildClientFromTransport function to check error handling coverage.
# Find the definition of the buildClientFromTransport function to understand its location and context.
ast-grep --lang go --pattern 'func buildClientFromTransport($_) $_ { $$$ }'
# Search for test functions that call buildClientFromTransport.
rg --type go 'buildClientFromTransport'
Length of output: 428
Script:
#!/bin/bash
# Description: Search for Go test files that might indirectly test the buildClientFromTransport function.
# List all Go test files in the repository.
fd --type f --extension go --exec basename {} \; | grep '_test.go'
# Search for any references to the buildClientFromTransport function in these test files.
rg --type go 'buildClientFromTransport' --glob '*_test.go'
Length of output: 5242
Tools
GitHub Check: codecov/patch
[warning] 198-199: core/metrics/otlp.go#L198-L199
Added lines #L198 - L199 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
core/metrics/otlp.go (2)
210-247: LGTM, but add comments for the exported functions.The addition of the option functions is a good design choice. It allows for clean and extensible configuration of the OTLP exporter. Exporting these functions enables flexibility in configuring the exporter from other packages. The functions modify the
transportOptionsstruct in a consistent way, appending the corresponding options for both HTTP and gRPC transports, which is a good practice for maintaining compatibility between the two transport types.However, static analysis tools have reported that these functions should have comments or be unexported.
Please add comments for the exported functions to improve code documentation and maintainability. For example:
// WithURL sets the endpoint URL for the OTLP exporter. func WithURL(url string) Option { // ... } // WithSecure enables or disables secure connection for the OTLP exporter. func WithSecure(secure bool) Option { // ... } // WithHeaders sets the headers for the OTLP exporter. func WithHeaders(headers string) Option { // ... }
54-54: LGTM, but use%wfor error wrapping.The error handling in the
Startmethod is generally good. It correctly returns an error if the creation of any exporter fails, which is important for error propagation.However, static analysis tools have reported a non-wrapping format verb for
fmt.Errorf.To improve error wrapping and preserve the original error context, use the
%wverb forfmt.Errorf. Apply this diff to fix the issue:-return fmt.Errorf("could not create exporter %d: %v", i, err) +return fmt.Errorf("could not create exporter %d: %w", i, err)Using
%wensures that the original error is wrapped properly and can be unwrapped later if needed.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- core/metrics/otlp.go (5 hunks)
Additional context used
GitHub Check: codecov/patch
core/metrics/otlp.go
[warning] 50-50: core/metrics/otlp.go#L50
Added line #L50 was not covered by tests
[warning] 55-55: core/metrics/otlp.go#L55
Added line #L55 was not covered by tests
[warning] 190-190: core/metrics/otlp.go#L190
Added line #L190 was not covered by tests
[warning] 194-195: core/metrics/otlp.go#L194-L195
Added lines #L194 - L195 were not covered by tests
Additional comments not posted (4)
core/metrics/otlp.go (4)
141-176: LGTM!The
makeOTLPExporterfunction is well-structured and handles errors appropriately. It uses the newbuildClientFromTransportfunction to create the OTLP client based on the transport type and options, which is a good abstraction. The error handling is comprehensive, returning meaningful error messages.
179-197: LGTM, but consider adding a unit test for error handling.The
buildClientFromTransportfunction is well-structured and handles errors appropriately. It uses a switch statement to create the appropriate client based on the transport type, which is a clean and extensible approach.However, static analysis indicates that the error handling for unknown transport types (lines 194-195) is not covered by tests.
Consider adding a unit test to cover this scenario and ensure the robustness of the function. For example:
func TestBuildClientFromTransport_UnknownTransport(t *testing.T) { _, err := buildClientFromTransport(otlpTransportType(0)) assert.Error(t, err) assert.Contains(t, err.Error(), "unknown transport type") }Tools
GitHub Check: codecov/patch
[warning] 190-190: core/metrics/otlp.go#L190
Added line #L190 was not covered by tests
[warning] 194-195: core/metrics/otlp.go#L194-L195
Added lines #L194 - L195 were not covered by tests
33-71: Excellent refactoring, but ensure test coverage for the multi-exporter logic.The refactoring of the
Startfunction is a significant improvement. It allows for the creation of multiple exporters based on environment variables, which enhances flexibility. The use of themakeOTLPExporterfunction abstracts the exporter creation logic, making the code more readable and maintainable. The integration of the multi-exporter into thebaseHandleris a good architectural decision.However, static analysis indicates that some lines are not covered by tests:
- Line 50: Breaking out of the loop when no more exporters are configured
- Line 55: Error handling when exporter creation fails
Consider adding unit tests to cover these scenarios and ensure the robustness of the multi-exporter logic and error handling. For example:
func TestStart_MultipleExporters(t *testing.T) { // Set up environment variables for multiple exporters // ... handler := NewOTLPMetricsHandler(buildInfo) err := handler.Start(context.Background()) assert.NoError(t, err) // Verify that multiple exporters were created // ... } func TestStart_ExporterCreationError(t *testing.T) { // Set up environment variables with invalid values // ... handler := NewOTLPMetricsHandler(buildInfo) err := handler.Start(context.Background()) assert.Error(t, err) assert.Contains(t, err.Error(), "could not create exporter") }Tools
GitHub Check: codecov/patch
[warning] 50-50: core/metrics/otlp.go#L50
Added line #L50 was not covered by tests
[warning] 55-55: core/metrics/otlp.go#L55
Added line #L55 was not covered by tests
117-120: LGTM!The addition of constants for maximum queue size and export batch size is a good practice. It provides clear configuration options for the exporter behavior. The modification of the constant names for the environment variables improves readability. The addition of the insecure mode environment variable constant is necessary for the new functionality.
Also applies to: 285-288
| func withSecure(secure bool) Option { | ||
| return func(o *transportOptions) error { | ||
| if secure { | ||
| tlsCreds := credentials.NewClientTLSFromCert(nil, "") | ||
| // note: you do not need to specify the tls creds for http, this happens automatically when https:// is used as the protocol scheme. | ||
| o.grpcOptions = append(o.grpcOptions, otlptracegrpc.WithTLSCredentials(tlsCreds)) | ||
|
|
||
| tlsConfig := &tls.Config{ | ||
| MinVersion: tls.VersionTLS12, | ||
| // RootCAs is nil, which means the default system root CAs are used | ||
| } | ||
| o.httpOptions = append(o.httpOptions, otlptracehttp.WithTLSClientConfig(tlsConfig)) | ||
| } else { | ||
| o.httpOptions = append(o.httpOptions, otlptracehttp.WithInsecure()) | ||
| o.grpcOptions = append(o.grpcOptions, otlptracegrpc.WithInsecure()) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
LGTM, but consider improving the TLS configuration.
The withSecure function is an important option for configuring secure connections for the OTLP exporter. It correctly sets the TLS credentials for gRPC transport and the TLS configuration for HTTP transport based on the secure parameter.
However, static analysis tools have reported that the TLS MinVersion is too low.
To ensure better security, consider setting a higher MinVersion for the TLS configuration. For example:
tlsConfig := &tls.Config{
MinVersion: tls.VersionTLS12,
// ...
}Using tls.VersionTLS12 or higher is recommended for secure connections.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation