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
Show file tree
Hide file tree
Changes from 5 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
46 changes: 46 additions & 0 deletions core/metrics/multiexporter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package metrics

import (
"context"
"fmt"

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

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) tracesdk.SpanExporter {
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 30 in core/metrics/multiexporter.go

View check run for this annotation

Codecov / codecov/patch

core/metrics/multiexporter.go#L25-L30

Added lines #L25 - L30 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 32 in core/metrics/multiexporter.go

View check run for this annotation

Codecov / codecov/patch

core/metrics/multiexporter.go#L32

Added line #L32 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 41 in core/metrics/multiexporter.go

View check run for this annotation

Codecov / codecov/patch

core/metrics/multiexporter.go#L40-L41

Added lines #L40 - L41 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
}


var _ tracesdk.SpanExporter = &multiExporter{}
68 changes: 54 additions & 14 deletions core/metrics/otlp.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -28,23 +28,46 @@
}

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))
// TODO: generalize this to allow for more than two exporters.
client, err := buildClientFromTransport(
transportFromString(
core.GetEnv(otlpTransportEnv, otlpTransportGRPC.String()),
),
)
if err != nil {
return fmt.Errorf("could not create client: %w", err)
}

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

View check run for this annotation

Codecov / codecov/patch

core/metrics/otlp.go#L38-L39

Added lines #L38 - L39 were not covered by tests

secondaryClient, err := buildClientFromTransport(
Copy link
Contributor

Choose a reason for hiding this comment

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

we should be able to tolerate cases where user doesn't define a secondary transport

transportFromString(
core.GetEnv(otlpTransportEnvSecondary, otlpTransportGRPC.String()),
),
)
if err != nil {
return fmt.Errorf("could not create secondary client: %w", err)

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

View check run for this annotation

Codecov / codecov/patch

core/metrics/otlp.go#L47

Added line #L47 was 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()))
secondaryExporter, err := otlptrace.New(ctx, secondaryClient)
if err != nil {
return fmt.Errorf("failed to create secondary otlp exporter: %w", err)
}

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

View check run for this annotation

Codecov / codecov/patch

core/metrics/otlp.go#L57-L58

Added lines #L57 - L58 were not covered by tests

multiExporter := NewMultiExporter(exporter, secondaryExporter)

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

// start the new parent
err = n.baseHandler.Start(ctx)
Expand Down Expand Up @@ -90,7 +113,8 @@
}

const (
otlpTransportEnv = "OTEL_EXPORTER_OTLP_TRANSPORT"
otlpTransportEnv = "OTEL_EXPORTER_OTLP_TRANSPORT"
otlpTransportEnvSecondary = "OTEL_EXPORTER_OTLP_TRANSPORT_SECONDARY"
)

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

func buildClientFromTransport(transport otlpTransportType) (otlptrace.Client, error) {
Copy link
Contributor

@trajan0x trajan0x Sep 5, 2024

Choose a reason for hiding this comment

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

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

switch transport {
case otlpTransportHTTP:
return otlptracehttp.NewClient(), nil

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

View check run for this annotation

Codecov / codecov/patch

core/metrics/otlp.go#L130-L131

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

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

View check run for this annotation

Codecov / codecov/patch

core/metrics/otlp.go#L134-L135

Added lines #L134 - L135 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

}
}

// transportFromString converts a string to a transport type.
// Defaults to http if the string is not recognized.
func transportFromString(transport string) otlpTransportType {
Expand All @@ -114,3 +149,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.
Expand Down
Loading