-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[jaeger-v2] Define an internal interface of storage v2 spanstore #5399
Merged
Merged
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
b10a1ee
add initial storage_v2/storage.proto API design
james-ryans 5ad4862
move storage_v2 proto to storage_v2/proto/storage.proto
james-ryans 1905b67
construct the storage_v2 spanstore interface structure
james-ryans 5b59349
Remove Close API from TraceWriter service
james-ryans 36c235a
Add empty test to storage_v2/spanstore/
james-ryans 61eba67
Merge branch 'main' into v2-storage-api
james-ryans 663324d
Clarify WriteTraces func description
james-ryans 40467b0
Use OTLP trace instead of Jaeger model
james-ryans ee73249
Define trace not found error in spanstore pkg
james-ryans 929288e
Fix Reader.FindTraces description typo
james-ryans 27451ec
Remove proto file from this PR
james-ryans c851fb2
Refactored from make fmt
james-ryans 67bd83b
Fix GetTrace func should return only one ptrace.Traces
james-ryans 04459ae
Change WriteTraces to accept pointer of traces
james-ryans bc09999
Add lifecycle methods to storage_v2.Factory
james-ryans 08fb5d7
Add empty test to storage_v2 pkg
james-ryans c6b2e27
Rename storage_v2.Factory to FactoryBase
james-ryans 6cbd5d8
Change spanstore Reader & Writer func back to accepts struct
james-ryans File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
// Copyright (c) 2024 The Jaeger Authors. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
package storage_v2 | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/jaegertracing/jaeger/pkg/testutils" | ||
) | ||
|
||
func TestMain(m *testing.M) { | ||
testutils.VerifyGoLeaks(m) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
// Copyright (c) 2024 The Jaeger Authors. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
package storage_v2 | ||
|
||
import ( | ||
"context" | ||
) | ||
|
||
// Factory is a general factory interface to be reused across different storage factories. | ||
// It lives within the OTEL collector extension component's lifecycle. | ||
// The Initialize and Close functions supposed to be called from the | ||
// OTEL component's Start and Shutdown functions. | ||
type FactoryBase interface { | ||
// Initialize performs internal initialization of the factory, | ||
// such as opening connections to the backend store. | ||
Initialize(ctx context.Context) error | ||
|
||
// Close closes the resources held by the factory | ||
Close(ctx context.Context) error | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
// Copyright (c) 2024 The Jaeger Authors. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
package spanstore | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/jaegertracing/jaeger/pkg/testutils" | ||
) | ||
|
||
func TestMain(m *testing.M) { | ||
testutils.VerifyGoLeaks(m) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
// Copyright (c) 2024 The Jaeger Authors. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
package spanstore | ||
|
||
import ( | ||
"github.com/jaegertracing/jaeger/storage_v2" | ||
) | ||
|
||
// Factory defines an interface for a factory that can create implementations of | ||
// different span storage components. | ||
type Factory interface { | ||
storage_v2.FactoryBase | ||
|
||
// CreateTraceReader creates a spanstore.Reader. | ||
CreateTraceReader() (Reader, error) | ||
|
||
// CreateTraceWriter creates a spanstore.Writer. | ||
CreateTraceWriter() (Writer, error) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
// Copyright (c) 2024 The Jaeger Authors. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
package spanstore | ||
|
||
import ( | ||
"context" | ||
"time" | ||
|
||
"go.opentelemetry.io/collector/pdata/pcommon" | ||
"go.opentelemetry.io/collector/pdata/ptrace" | ||
|
||
spanstore_v1 "github.com/jaegertracing/jaeger/storage/spanstore" | ||
) | ||
|
||
// ErrTraceNotFound is returned by Reader's GetTrace if no data is found for given trace ID. | ||
var ErrTraceNotFound = spanstore_v1.ErrTraceNotFound | ||
|
||
// Reader finds and loads traces and other data from storage. | ||
type Reader interface { | ||
// GetTrace retrieves the trace with a given id. | ||
// | ||
// If no spans are stored for this trace, it returns ErrTraceNotFound. | ||
yurishkuro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
GetTrace(ctx context.Context, traceID pcommon.TraceID) (ptrace.Traces, error) | ||
|
||
// GetServices returns all service names known to the backend from spans | ||
// within its retention period. | ||
GetServices(ctx context.Context) ([]string, error) | ||
|
||
// GetOperations returns all operation names for a given service | ||
// known to the backend from spans within its retention period. | ||
GetOperations(ctx context.Context, query OperationQueryParameters) ([]Operation, error) | ||
|
||
// FindTraces returns all traces matching query parameters. There's currently | ||
// an implementation-dependent ambiguity whether all query filters (such as | ||
// multiple tags) must apply to the same span within a trace, or can be satisfied | ||
// by different spans. | ||
// | ||
// If no matching traces are found, the function returns (nil, nil). | ||
FindTraces(ctx context.Context, query TraceQueryParameters) ([]ptrace.Traces, error) | ||
|
||
// FindTraceIDs does the same search as FindTraces, but returns only the list | ||
// of matching trace IDs. | ||
// | ||
// If no matching traces are found, the function returns (nil, nil). | ||
FindTraceIDs(ctx context.Context, query TraceQueryParameters) ([]pcommon.TraceID, error) | ||
} | ||
|
||
// TraceQueryParameters contains parameters of a trace query. | ||
type TraceQueryParameters struct { | ||
ServiceName string | ||
OperationName string | ||
Tags map[string]string | ||
StartTimeMin time.Time | ||
StartTimeMax time.Time | ||
DurationMin time.Duration | ||
DurationMax time.Duration | ||
NumTraces int | ||
} | ||
|
||
// OperationQueryParameters contains parameters of query operations, empty spanKind means get operations for all kinds of span. | ||
type OperationQueryParameters struct { | ||
ServiceName string | ||
SpanKind string | ||
} | ||
|
||
// Operation contains operation name and span kind | ||
type Operation struct { | ||
Name string | ||
SpanKind string | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
// Copyright (c) 2024 The Jaeger Authors. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
package spanstore | ||
|
||
import ( | ||
"context" | ||
|
||
"go.opentelemetry.io/collector/pdata/ptrace" | ||
) | ||
|
||
// Writer writes spans to storage. | ||
type Writer interface { | ||
// WriteTrace writes a batch of spans to storage. Idempotent. | ||
// Implementations are not required to support atomic transactions, | ||
// so if any of the spans fail to be written an error is returned. | ||
// Compatible with OTLP Exporter API. | ||
WriteTraces(ctx context.Context, td ptrace.Traces) error | ||
yurishkuro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cf open-telemetry/opentelemetry-collector#9324
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've read through the discussions and the latest approach suggested that the
Start
function shouldn't throw error anymore and uses theTelemetrySettings.ReportStatus
to reportStatusFatalError
to halt the startup asynchronously, cmiiw.I'm thinking that any changes to the component lifecycles should be handled by the Jaeger storage extension, we can capture the errors from the storage factory in the extension and report them instead, so the storage factory interface should not be affected by it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, this will be similar to what jaeger-v1 does today. However, jaeger-v1 has a failure mode today where if the db is not ready for any reason when the binary starts (even though the Jaeger configuration pointing to the db is correct), Jaeger fails to start and the binary exists. That's not how a component of a distributed system technically should be operating, i.e. a db temporarily not available at start-up vs. becoming temporarily not available during normal operation should not be treated as different conditions. We don't exit the binary if we lose a connection later, so why do we kill it if we cannot establish the connection during Init()?
If we were to fix this, we need to establish a different expectation from storage implementations, and this is where async runtime status reporting instead of returning an error from Init is coming from. This open-telemetry/opentelemetry-collector#9324 (comment) has a good explanation.