Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SLT-141] feat(metrics): multiple exports #3099

Merged
merged 33 commits into from
Sep 11, 2024
Merged
Changes from 11 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
7ba8a43
initial
golangisfun123 Sep 3, 2024
9632578
done
golangisfun123 Sep 4, 2024
c762c4c
add lint
golangisfun123 Sep 4, 2024
3c15801
Merge branch 'master' into multiple-exports
trajan0x Sep 5, 2024
829812b
[goreleaser]
trajan0x Sep 5, 2024
e831fe9
different environment variables
golangisfun123 Sep 5, 2024
68def3c
rework logic, make primary/secondary optional, make code clearer, upd…
golangisfun123 Sep 5, 2024
51284b9
clearer code
golangisfun123 Sep 5, 2024
6bcd3ab
interfacify it better
golangisfun123 Sep 5, 2024
38e924b
lint
golangisfun123 Sep 5, 2024
f8a2048
[goreleaser]
golangisfun123 Sep 5, 2024
c163704
add test
trajan0x Sep 10, 2024
89e996b
cleanup
trajan0x Sep 10, 2024
780b4c5
actually suport multi exports
trajan0x Sep 10, 2024
1d82704
address https://github.com/synapsecns/sanguine/pull/3099/files#r17513…
trajan0x Sep 10, 2024
08fa2a4
fix function call [goreleaser]
golangisfun123 Sep 10, 2024
1c63c19
warnings and uneeded err check
golangisfun123 Sep 10, 2024
d08315e
more accurate comment
golangisfun123 Sep 10, 2024
1d670a0
[goreleaser]
golangisfun123 Sep 10, 2024
d6ee535
cleanup
trajan0x Sep 11, 2024
48f2afc
resolve merge conflicts
trajan0x Sep 11, 2024
4db816f
header bump [goreleaser]
trajan0x Sep 11, 2024
3923d42
Merge branch 'master' into multiple-exports
trajan0x Sep 11, 2024
bad4071
insecure mode [goreleaser]
trajan0x Sep 11, 2024
f8e240d
secure [goreleaser]
trajan0x Sep 11, 2024
a19300c
set default exporter to otlpgrpc again
trajan0x Sep 11, 2024
bfd1d7b
test
trajan0x Sep 11, 2024
d5e69f7
Revert "test"
trajan0x Sep 11, 2024
ceeddbc
cleanup
trajan0x Sep 11, 2024
4c6825a
cleanup [goreleaser]
trajan0x Sep 11, 2024
732f5fa
Merge branch 'multiple-exports' of https://github.com/synapsecns/sang…
trajan0x Sep 11, 2024
8a04f9e
paralellize exports [goreleaser]
trajan0x Sep 11, 2024
0e3a5ae
url
trajan0x Sep 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion core/metrics/README.md
Original file line number Diff line number Diff line change
@@ -12,8 +12,13 @@ There's also a `NAME_PREFIX` environment variable that will prefix all the metri

## OTLP

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. Key ones to note are:
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` |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_PRIMARY is listed with a default of true, which is unusual for a URL.
  • OTEL_EXPORTER_OTLP_TRANSPORT_SECONDARY is listed with a default of 8080, 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.


## Jaeger

57 changes: 57 additions & 0 deletions core/metrics/multiexporter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package metrics

import (
"context"
"fmt"

"go.opentelemetry.io/otel/exporters/otlp/otlptrace"
tracesdk "go.opentelemetry.io/otel/sdk/trace"
)

// MultiExporter is an interface that allows exporting spans to multiple OTLP trace exporters.
type MultiExporter interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should have a test

tracesdk.SpanExporter
AddExporter(exporter *otlptrace.Exporter)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}

type multiExporter struct {
exporters []*otlptrace.Exporter
}

// NewMultiExporter creates a new multi exporter that forwards spans to multiple OTLP trace exporters.
// It takes in one or more otlptrace.Exporter instances and ensures that spans are sent to all of them.
// This is useful when you need to send trace data to multiple backends or endpoints.
func NewMultiExporter(exporters ...*otlptrace.Exporter) MultiExporter {
return &multiExporter{
exporters: exporters,
}
}

// 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)
}

Check warning on line 36 in core/metrics/multiexporter.go

Codecov / codecov/patch

core/metrics/multiexporter.go#L31-L36

Added lines #L31 - L36 were not covered by tests
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

}
return nil

Check warning on line 38 in core/metrics/multiexporter.go

Codecov / codecov/patch

core/metrics/multiexporter.go#L38

Added line #L38 was not covered by tests
}
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@golangisfun123 thoughts in desirable behavior?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider collecting errors from all exporters before returning.

The current implementation of ExportSpans 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.

As suggested in the existing review comments, 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(ctx context.Context, ss []tracesdk.ReadOnlySpan) error {
+	var errs []error
 	for _, exporter := range m.exporters {
 		err := exporter.ExportSpans(ctx, ss)
 		if err != nil {
-			return fmt.Errorf("could not export spans: %w", err)
+			errs = append(errs, err)
 		}
 	}
+	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.

Suggested change
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
}
func (m *multiExporter) ExportSpans(ctx context.Context, ss []tracesdk.ReadOnlySpan) error {
var errs []error
for _, exporter := range m.exporters {
err := exporter.ExportSpans(ctx, ss)
if err != nil {
errs = append(errs, err)
}
}
if len(errs) > 0 {
return fmt.Errorf("could not export spans to some exporters: %v", errs)
}
return nil
}


// 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)
}

Check warning on line 47 in core/metrics/multiexporter.go

Codecov / codecov/patch

core/metrics/multiexporter.go#L46-L47

Added lines #L46 - L47 were not covered by tests
}
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in the existing review comments, 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 {
+	var errs []error
 	for _, exporter := range m.exporters {
 		err := exporter.Shutdown(ctx)
 		if err != nil {
-			return fmt.Errorf("could not stop exporter: %w", err)
+			errs = append(errs, err)
 		}
 	}
+	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.

Suggested change
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
}
func (m *multiExporter) Shutdown(ctx context.Context) error {
var errs []error
for _, exporter := range m.exporters {
err := exporter.Shutdown(ctx)
if err != nil {
errs = append(errs, err)
}
}
if len(errs) > 0 {
return fmt.Errorf("could not stop some exporters: %v", errs)
}
return nil
}


// AddExporter adds an exporter to the multi exporter.
func (m *multiExporter) AddExporter(exporter *otlptrace.Exporter) {
m.exporters = append(m.exporters, exporter)
}

var _ tracesdk.SpanExporter = &multiExporter{}
90 changes: 76 additions & 14 deletions core/metrics/otlp.go
Original file line number Diff line number Diff line change
@@ -3,15 +3,15 @@
import (
"context"
"fmt"
"strings"
"time"

"github.com/synapsecns/sanguine/core"
"github.com/synapsecns/sanguine/core/config"
"go.opentelemetry.io/otel/exporters/otlp/otlptrace"
"go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc"
"go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp"
tracesdk "go.opentelemetry.io/otel/sdk/trace"
"os"
"strings"
"time"
)

type otlpHandler struct {
@@ -28,23 +28,51 @@
}

func (n *otlpHandler) Start(ctx context.Context) (err error) {
var client otlptrace.Client
transport := transportFromString(core.GetEnv(otlpTransportEnv, otlpTransportGRPC.String()))
switch transport {
case otlpTransportHTTP:
client = otlptracehttp.NewClient()
case otlpTransportGRPC:
client = otlptracegrpc.NewClient()
default:
return fmt.Errorf("unknown transport type: %s", os.Getenv(otlpTransportEnv))
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)
}

Check warning on line 38 in core/metrics/otlp.go

Codecov / codecov/patch

core/metrics/otlp.go#L35-L38

Added lines #L35 - L38 were not covered by tests
} 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)
}

Check warning on line 44 in core/metrics/otlp.go

Codecov / codecov/patch

core/metrics/otlp.go#L43-L44

Added lines #L43 - L44 were not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


secondaryClient, err = getClient(otlpTransportEnvSecondary)
if err != nil {
return fmt.Errorf("could not create secondary client: %w", err)
}

Check warning on line 49 in core/metrics/otlp.go

Codecov / codecov/patch

core/metrics/otlp.go#L48-L49

Added lines #L48 - L49 were not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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

}

exporter, err := otlptrace.New(ctx, client)
if err != nil {
return fmt.Errorf("failed to create otlp exporter: %w", err)
}

n.baseHandler = newBaseHandler(n.buildInfo, tracesdk.WithBatcher(exporter, tracesdk.WithMaxQueueSize(1000000), tracesdk.WithMaxExportBatchSize(2000)), tracesdk.WithSampler(tracesdk.AlwaysSample()))
// 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)
}

Check warning on line 63 in core/metrics/otlp.go

Codecov / codecov/patch

core/metrics/otlp.go#L62-L63

Added lines #L62 - L63 were not covered by tests
multiExporter.AddExporter(secondaryExporter)
}

Check warning on line 65 in core/metrics/otlp.go

Codecov / codecov/patch

core/metrics/otlp.go#L65

Added line #L65 was not covered by tests

n.baseHandler = newBaseHandler(
n.buildInfo,
tracesdk.WithBatcher(
multiExporter,
tracesdk.WithMaxQueueSize(defaultMaxQueueSize),
tracesdk.WithMaxExportBatchSize(defaultMaxExportBatch),
),
tracesdk.WithSampler(tracesdk.AlwaysSample()),
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// start the new parent
err = n.baseHandler.Start(ctx)
@@ -90,7 +118,9 @@
}

const (
otlpTransportEnv = "OTEL_EXPORTER_OTLP_TRANSPORT"
otlpTransportEnvDefault = "OTEL_EXPORTER_OTLP_TRANSPORT"
otlpTransportEnvPrimary = "OTEL_EXPORTER_OTLP_TRANSPORT_PRIMARY"
otlpTransportEnvSecondary = "OTEL_EXPORTER_OTLP_TRANSPORT_SECONDARY"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

)

//go:generate go run golang.org/x/tools/cmd/stringer -type=otlpTransportType -linecomment
@@ -101,6 +131,33 @@
otlpTransportGRPC // grpc
)

// getClient creates a new OTLP client based on the transport type and url.
func getClient(transportEnv string) (otlptrace.Client, error) {
transport := transportFromString(core.GetEnv(transportEnv, otlpTransportHTTP.String()))
url := core.GetEnv(transportEnv, "")

return buildClientFromTransport(
transport,
url,
)
}

// 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

Check warning on line 153 in core/metrics/otlp.go

Codecov / codecov/patch

core/metrics/otlp.go#L152-L153

Added lines #L152 - L153 were not covered by tests
case otlpTransportGRPC:
return otlptracegrpc.NewClient(otlptracegrpc.WithEndpointURL(url)), nil
default:
return nil, fmt.Errorf("unknown transport type: %s", transport.String())

Check warning on line 157 in core/metrics/otlp.go

Codecov / codecov/patch

core/metrics/otlp.go#L156-L157

Added lines #L156 - L157 were not covered by tests
Comment on lines +194 to +195
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// transportFromString converts a string to a transport type.
// Defaults to http if the string is not recognized.
func transportFromString(transport string) otlpTransportType {
@@ -114,3 +171,8 @@
// (see uber's go stye guide for details)
return otlpTransportType(0)
}

const (
defaultMaxQueueSize = 1000000
defaultMaxExportBatch = 2000
)
5 changes: 4 additions & 1 deletion core/metrics/rookout.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
//go:build go1.16 && !go1.23

package metrics

import (
"os"

rookout "github.com/Rookout/GoSDK"
"github.com/Rookout/GoSDK/pkg/config"
"github.com/synapsecns/sanguine/core"
synconfig "github.com/synapsecns/sanguine/core/config"
"github.com/synapsecns/sanguine/core/metrics/internal"
"os"
)

// DefaultGitRepo is the default git repo for sanguine.
Loading