Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
ad0de07
Create Query Service V2 To Operate on OTEL Data Model
mahadzaryab1 Dec 11, 2024
24278bb
Merge branch 'main' into query-service-v2
mahadzaryab1 Dec 24, 2024
41f1d51
Merge branch 'main' into query-service-v2
mahadzaryab1 Dec 24, 2024
a33ab6c
Address Build Failures And Feedback
mahadzaryab1 Dec 24, 2024
ffad629
Fix ArchiveTrace And GetTraces To Operate On Iterators
mahadzaryab1 Dec 24, 2024
1d721ac
Change Adjuster To Work On Seq
mahadzaryab1 Dec 24, 2024
a6df503
Fix Error Capture
mahadzaryab1 Dec 24, 2024
60a875a
Address Feedback From PR Review
mahadzaryab1 Dec 25, 2024
d31a5e5
Merge branch 'main' into query-service-v2
mahadzaryab1 Dec 25, 2024
2c7a848
Merge branch 'main' into query-service-v2
mahadzaryab1 Dec 29, 2024
d32ba48
Run Linter
mahadzaryab1 Dec 29, 2024
ecab080
Update Query Service To Perform Adjustments
mahadzaryab1 Dec 30, 2024
9ee2d38
Fix Signature of ArchiveTrace Function
mahadzaryab1 Dec 30, 2024
57ef9e7
Add Some Unit Tests
mahadzaryab1 Dec 30, 2024
40d9a68
Add Missing License
mahadzaryab1 Dec 30, 2024
32491f5
Adjust Archive Trace
mahadzaryab1 Dec 30, 2024
3d6af5f
Aggregate Traces
mahadzaryab1 Dec 30, 2024
ab95ab7
Remove TODO Comment
mahadzaryab1 Dec 30, 2024
6737ca9
Merge branch 'main' into query-service-v2
mahadzaryab1 Dec 30, 2024
005d8d7
Use SpanIter
mahadzaryab1 Dec 30, 2024
7171785
Add Proceed Check
mahadzaryab1 Dec 30, 2024
e5a4b1b
Address Feedback From PR Review
mahadzaryab1 Dec 30, 2024
f887d09
Create Receive Traces Helper Function
mahadzaryab1 Dec 30, 2024
959d2d4
Add Unit Tests For Get Traces
mahadzaryab1 Dec 30, 2024
d468249
Fix Lint
mahadzaryab1 Dec 30, 2024
f6d157e
Add Unit Tests For Find Traces
mahadzaryab1 Dec 30, 2024
d2f1c95
Add Test For Archive Trace Writer Error
mahadzaryab1 Dec 31, 2024
6f69d63
Add Test For Archive Trace Writer Success
mahadzaryab1 Dec 31, 2024
9250285
Write Test For Get Traces In Archive Storage
mahadzaryab1 Dec 31, 2024
1e0c5b3
Move Tests To Separate Package To Simplify Naming
mahadzaryab1 Dec 31, 2024
c14fc4f
Fix Comment
mahadzaryab1 Dec 31, 2024
73fbb19
Add Test For Error In Get Trace
mahadzaryab1 Dec 31, 2024
5020f89
Use Flatten With Errors
mahadzaryab1 Dec 31, 2024
e29eafe
Add Test For Error In Get Trace Reader
mahadzaryab1 Dec 31, 2024
2c716bf
Cleanup Tests
mahadzaryab1 Dec 31, 2024
28e5fd0
Combine Tests For Brevity
mahadzaryab1 Dec 31, 2024
5ae23f2
Fix Lint
mahadzaryab1 Dec 31, 2024
a79bcac
Merge branch 'main' into query-service-v2
mahadzaryab1 Dec 31, 2024
5636427
Move Query Service To V2 Package
mahadzaryab1 Dec 31, 2024
529bcda
Fix Lint
mahadzaryab1 Dec 31, 2024
806a265
Drop V2 Suffix
mahadzaryab1 Dec 31, 2024
f163d55
Merge branch 'main' into query-service-v2
mahadzaryab1 Dec 31, 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
6 changes: 0 additions & 6 deletions cmd/query/app/querysvc/query_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,6 @@ import (
"github.com/jaegertracing/jaeger/storage_v2/v1adapter"
)

var errNoArchiveSpanStorage = errors.New("archive span storage was not configured")

const (
defaultMaxClockSkewAdjust = time.Second
)

// QueryServiceOptions has optional members of QueryService
type QueryServiceOptions struct {
ArchiveSpanReader spanstore.Reader
Expand Down
175 changes: 175 additions & 0 deletions cmd/query/app/querysvc/query_service_v2.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
// Copyright (c) 2024 The Jaeger Authors.
// SPDX-License-Identifier: Apache-2.0

package querysvc

import (
"context"
"errors"
"time"

"go.opentelemetry.io/collector/pdata/pcommon"
"go.opentelemetry.io/collector/pdata/ptrace"

"github.com/jaegertracing/jaeger/cmd/query/app/querysvc/adjuster"
"github.com/jaegertracing/jaeger/model"
"github.com/jaegertracing/jaeger/pkg/iter"
"github.com/jaegertracing/jaeger/storage_v2/depstore"
"github.com/jaegertracing/jaeger/storage_v2/tracestore"
)

var errNoArchiveSpanStorage = errors.New("archive span storage was not configured")

const (
defaultMaxClockSkewAdjust = time.Second
)

// QueryServiceOptions has optional members of QueryService
type QueryServiceOptionsV2 struct {
ArchiveTraceReader tracestore.Reader
ArchiveTraceWriter tracestore.Writer
Adjuster adjuster.Adjuster
}

// StorageCapabilities is a feature flag for query service
type StorageCapabilitiesV2 struct {
ArchiveStorage bool `json:"archiveStorage"`
// TODO: Maybe add metrics Storage here
// SupportRegex bool
// SupportTagFilter bool
Copy link
Collaborator Author

@mahadzaryab1 mahadzaryab1 Dec 31, 2024

Choose a reason for hiding this comment

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

@yurishkuro do you have the context for these items? do we still want to do them?

}

// QueryService contains span utils required by the query-service.
type QueryServiceV2 struct {
traceReader tracestore.Reader
dependencyReader depstore.Reader
options QueryServiceOptionsV2
}

// NewQueryService returns a new QueryService.
func NewQueryServiceV2(
traceReader tracestore.Reader,
dependencyReader depstore.Reader,
options QueryServiceOptionsV2,
) *QueryServiceV2 {
qsvc := &QueryServiceV2{
traceReader: traceReader,
dependencyReader: dependencyReader,
options: options,
}

if qsvc.options.Adjuster == nil {
qsvc.options.Adjuster = adjuster.Sequence(adjuster.StandardAdjusters(defaultMaxClockSkewAdjust)...)
}
return qsvc

Check warning on line 64 in cmd/query/app/querysvc/query_service_v2.go

View check run for this annotation

Codecov / codecov/patch

cmd/query/app/querysvc/query_service_v2.go#L54-L64

Added lines #L54 - L64 were not covered by tests
}

// GetTrace is the queryService implementation of tracestore.Reader.GetTrace
func (qs QueryServiceV2) GetTraces(ctx context.Context, traceIDs ...tracestore.GetTraceParams) iter.Seq2[[]ptrace.Traces, error] {
getTracesIter := qs.traceReader.GetTraces(ctx, traceIDs...)
return func(yield func([]ptrace.Traces, error) bool) {
foundTraceIDs := make(map[pcommon.TraceID]struct{})
Copy link
Member

Choose a reason for hiding this comment

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

you can move code from L94-L112 into a helper function

receiveTraces(
    seq iter.Seq2[[]ptrace.Traces, error], 
    yield func()...,
) (visited make(map[pcommon.TraceID]struct{}), proceed bool)

and reuse it for main reader, archive reader, and in FindTraces (which is currently incorrect as it does not pre-aggreate the chunks).

getTracesIter(func(traces []ptrace.Traces, err error) bool {
if err != nil {
return yield(nil, err)
}
for _, trace := range traces {
resources := trace.ResourceSpans()
for i := 0; i < resources.Len(); i++ {
scopes := resources.At(i).ScopeSpans()
for j := 0; j < scopes.Len(); j++ {
spans := scopes.At(j).Spans()
for k := 0; k < spans.Len(); k++ {
span := spans.At(k)
foundTraceIDs[span.TraceID()] = struct{}{}
}

Check warning on line 85 in cmd/query/app/querysvc/query_service_v2.go

View check run for this annotation

Codecov / codecov/patch

cmd/query/app/querysvc/query_service_v2.go#L68-L85

Added lines #L68 - L85 were not covered by tests
}
}
}
return yield(traces, nil)

Check warning on line 89 in cmd/query/app/querysvc/query_service_v2.go

View check run for this annotation

Codecov / codecov/patch

cmd/query/app/querysvc/query_service_v2.go#L89

Added line #L89 was not covered by tests
})
if qs.options.ArchiveTraceReader != nil {
Copy link
Member

Choose a reason for hiding this comment

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

somewhere you should be checking if an error was already encountered or the caller's yield function returned false, such that you don't continue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yurishkuro there's a check on line 97 - or are you talking about a different check?

Copy link
Member

Choose a reason for hiding this comment

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

More comprehensively - no matter what happens inside your lambda function once you get to this point you still continue, even though the caller's yield() may have returned false. You need to keep a bool flag outside of lambda.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yurishkuro Thanks for the explanation. I made this change - let me know if it makes sense.

missingTraceIDs := []tracestore.GetTraceParams{}
for _, id := range traceIDs {
if _, found := foundTraceIDs[id.TraceID]; !found {
missingTraceIDs = append(missingTraceIDs, id)
}

Check warning on line 96 in cmd/query/app/querysvc/query_service_v2.go

View check run for this annotation

Codecov / codecov/patch

cmd/query/app/querysvc/query_service_v2.go#L91-L96

Added lines #L91 - L96 were not covered by tests
}
if len(missingTraceIDs) > 0 {
qs.options.ArchiveTraceReader.GetTraces(ctx, missingTraceIDs...)(yield)
}

Check warning on line 100 in cmd/query/app/querysvc/query_service_v2.go

View check run for this annotation

Codecov / codecov/patch

cmd/query/app/querysvc/query_service_v2.go#L98-L100

Added lines #L98 - L100 were not covered by tests
}
}
}

// GetServices is the queryService implementation of tracestore.Reader.GetServices
func (qs QueryServiceV2) GetServices(ctx context.Context) ([]string, error) {
return qs.traceReader.GetServices(ctx)

Check warning on line 107 in cmd/query/app/querysvc/query_service_v2.go

View check run for this annotation

Codecov / codecov/patch

cmd/query/app/querysvc/query_service_v2.go#L106-L107

Added lines #L106 - L107 were not covered by tests
}

// GetOperations is the queryService implementation of tracestore.Reader.GetOperations
func (qs QueryServiceV2) GetOperations(
ctx context.Context,
query tracestore.OperationQueryParameters,
) ([]tracestore.Operation, error) {
return qs.traceReader.GetOperations(ctx, query)

Check warning on line 115 in cmd/query/app/querysvc/query_service_v2.go

View check run for this annotation

Codecov / codecov/patch

cmd/query/app/querysvc/query_service_v2.go#L114-L115

Added lines #L114 - L115 were not covered by tests
}

// FindTraces is the queryService implementation of tracestore.Reader.FindTraces
func (qs QueryServiceV2) FindTraces(ctx context.Context, query tracestore.TraceQueryParams) iter.Seq2[[]ptrace.Traces, error] {
return qs.traceReader.FindTraces(ctx, query)

Check warning on line 120 in cmd/query/app/querysvc/query_service_v2.go

View check run for this annotation

Codecov / codecov/patch

cmd/query/app/querysvc/query_service_v2.go#L119-L120

Added lines #L119 - L120 were not covered by tests
}

// ArchiveTrace is the queryService utility to archive traces.
func (qs QueryServiceV2) ArchiveTrace(ctx context.Context, traceID pcommon.TraceID) error {
if qs.options.ArchiveTraceWriter == nil {
return errNoArchiveSpanStorage
}
getTracesIter := qs.GetTraces(ctx, tracestore.GetTraceParams{TraceID: traceID})
var archiveErr error
getTracesIter(func(traces []ptrace.Traces, err error) bool {
if err != nil {
archiveErr = err
return false
}
for _, trace := range traces {
err = qs.options.ArchiveTraceWriter.WriteTraces(ctx, trace)
if err != nil {
archiveErr = err
return false
}

Check warning on line 140 in cmd/query/app/querysvc/query_service_v2.go

View check run for this annotation

Codecov / codecov/patch

cmd/query/app/querysvc/query_service_v2.go#L124-L140

Added lines #L124 - L140 were not covered by tests
}
return true

Check warning on line 142 in cmd/query/app/querysvc/query_service_v2.go

View check run for this annotation

Codecov / codecov/patch

cmd/query/app/querysvc/query_service_v2.go#L142

Added line #L142 was not covered by tests
})
return archiveErr

Check warning on line 144 in cmd/query/app/querysvc/query_service_v2.go

View check run for this annotation

Codecov / codecov/patch

cmd/query/app/querysvc/query_service_v2.go#L144

Added line #L144 was not covered by tests
}

// Adjust applies adjusters to the trace.
func (qs QueryServiceV2) Adjust(tracesIter iter.Seq[[]ptrace.Traces]) {
tracesIter(func(traces []ptrace.Traces) bool {
for _, trace := range traces {
qs.options.Adjuster.Adjust(trace)
Copy link
Member

Choose a reason for hiding this comment

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

you cannot assume here that trace is a full trace, you need to clump consecutive chunks if they are for the same trace ID. We can implement a helper function for that that will take Seq[[]ptrace.Traces] and return Seq[ptrace.Traces] where each item is a full trace. Similar to what I was suggesting in https://github.com/jaegertracing/jaeger/pull/6388/files#r1894703201

}
return true

Check warning on line 153 in cmd/query/app/querysvc/query_service_v2.go

View check run for this annotation

Codecov / codecov/patch

cmd/query/app/querysvc/query_service_v2.go#L148-L153

Added lines #L148 - L153 were not covered by tests
})
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yurishkuro Did I understand correctly in that this is what we wanted here? Or did you mean that we should change the underlying adjusters themselves to work on Seq?

Copy link
Member

Choose a reason for hiding this comment

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

yes, you understood correctly. However, because adjusting needs to re-arrange the data I think this func should return Seq[ptrace.Traces] where each item is fully adjusted trace.


// GetDependencies implements depstore.Reader.GetDependencies
func (qs QueryServiceV2) GetDependencies(ctx context.Context, endTs time.Time, lookback time.Duration) ([]model.DependencyLink, error) {
return qs.dependencyReader.GetDependencies(ctx, depstore.QueryParameters{
StartTime: endTs.Add(-lookback),
EndTime: endTs,
})

Check warning on line 162 in cmd/query/app/querysvc/query_service_v2.go

View check run for this annotation

Codecov / codecov/patch

cmd/query/app/querysvc/query_service_v2.go#L158-L162

Added lines #L158 - L162 were not covered by tests
}

// GetCapabilities returns the features supported by the query service.
func (qs QueryServiceV2) GetCapabilities() StorageCapabilities {
return StorageCapabilities{
ArchiveStorage: qs.options.hasArchiveStorage(),
}

Check warning on line 169 in cmd/query/app/querysvc/query_service_v2.go

View check run for this annotation

Codecov / codecov/patch

cmd/query/app/querysvc/query_service_v2.go#L166-L169

Added lines #L166 - L169 were not covered by tests
}

// hasArchiveStorage returns true if archive storage reader/writer are initialized.
func (opts *QueryServiceOptionsV2) hasArchiveStorage() bool {
return opts.ArchiveTraceReader != nil && opts.ArchiveTraceWriter != nil

Check warning on line 174 in cmd/query/app/querysvc/query_service_v2.go

View check run for this annotation

Codecov / codecov/patch

cmd/query/app/querysvc/query_service_v2.go#L173-L174

Added lines #L173 - L174 were not covered by tests
}
Loading