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.

Sign up for GitHub

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

[query] Truncate search results to prevent OOM on jaeger-query. #1202

Closed
wants to merge 3 commits into from
Closed

[query] Truncate search results to prevent OOM on jaeger-query. #1202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2008414
[Storage] Truncate search results to prevent OOM on jaeger-query.
Nov 20, 2018
e05a4d4
Adding num-spans limit
Nov 30, 2018
a509e7d
Limit span count in ES queries
Dec 9, 2018
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
13 changes: 13 additions & 0 deletions cmd/query/app/query_parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,14 @@ import (

const (
defaultQueryLimit = 100
defaultSpansLimit = 10000
Copy link
Member

Choose a reason for hiding this comment

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

there was no default previously, we should not introduce one. I.e. default should be 0, which is == unlimited.

Copy link
Member

Choose a reason for hiding this comment

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

see the very last comment


operationParam = "operation"
tagParam = "tag"
tagsParam = "tags"
startTimeParam = "start"
limitParam = "limit"
limitSpansParam = "limitSpans"
Copy link
Member

Choose a reason for hiding this comment

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

s/limitSpans/maxSpansPerTrace/

Copy link
Member

Choose a reason for hiding this comment

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

see the very last comment

minDurationParam = "minDuration"
maxDurationParam = "maxDuration"
serviceParam = "service"
Expand Down Expand Up @@ -105,6 +107,16 @@ func (p *queryParser) parse(r *http.Request) (*traceQueryParameters, error) {
limit = int(limitParsed)
}

limitSpansParam := r.FormValue(limitSpansParam)
limitSpans := defaultSpansLimit
Copy link
Member

Choose a reason for hiding this comment

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

scope the string var to the if statement:

var maxSpansPerTrace int
if maxSpansParam := r.FormValue(maxSpansPerTraceParam); maxSpansParam != "" {
}

if limitSpansParam != "" {
limitParsed, err := strconv.ParseInt(limitSpansParam, 10, 32)
if err != nil {
return nil, err
}
limitSpans = int(limitParsed)
}

minDuration, err := p.parseDuration(minDurationParam, r)
if err != nil {
return nil, err
Expand Down Expand Up @@ -137,6 +149,7 @@ func (p *queryParser) parse(r *http.Request) (*traceQueryParameters, error) {
StartTimeMax: endTime,
Tags: tags,
NumTraces: limit,
NumSpans: limitSpans,
DurationMin: minDuration,
DurationMax: maxDuration,
},
Expand Down
7 changes: 7 additions & 0 deletions cmd/query/app/query_parser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ func TestParseTraceQuery(t *testing.T) {
StartTimeMin: time.Unix(0, 0),
StartTimeMax: time.Unix(0, 0),
NumTraces: 200,
NumSpans: 10000,
Tags: map[string]string{"k": "v", "x": "y"},
},
},
Expand All @@ -68,6 +69,7 @@ func TestParseTraceQuery(t *testing.T) {
StartTimeMin: time.Unix(0, 0),
StartTimeMax: time.Unix(0, 0),
NumTraces: 200,
NumSpans: 10000,
Tags: map[string]string{"k": "v", "x": "y"},
},
},
Expand All @@ -81,6 +83,7 @@ func TestParseTraceQuery(t *testing.T) {
StartTimeMin: time.Unix(0, 0),
StartTimeMax: time.Unix(0, 0),
NumTraces: 200,
NumSpans: 10000,
Tags: map[string]string{"k": "v", "x": "y"},
},
},
Expand All @@ -93,6 +96,7 @@ func TestParseTraceQuery(t *testing.T) {
StartTimeMin: time.Unix(0, 0),
StartTimeMax: time.Unix(0, 0),
NumTraces: 200,
NumSpans: 10000,
DurationMin: 10 * time.Second,
DurationMax: 20 * time.Second,
Tags: make(map[string]string),
Expand All @@ -107,6 +111,7 @@ func TestParseTraceQuery(t *testing.T) {
StartTimeMin: time.Unix(0, 0),
StartTimeMax: time.Unix(0, 0),
NumTraces: 200,
NumSpans: 10000,
DurationMin: 10 * time.Second,
Tags: make(map[string]string),
},
Expand All @@ -117,6 +122,7 @@ func TestParseTraceQuery(t *testing.T) {
&traceQueryParameters{
TraceQueryParameters: spanstore.TraceQueryParameters{
NumTraces: 100,
NumSpans: 10000,
StartTimeMin: timeNow,
StartTimeMax: timeNow,
Tags: make(map[string]string),
Expand All @@ -133,6 +139,7 @@ func TestParseTraceQuery(t *testing.T) {
StartTimeMin: time.Unix(0, 0),
StartTimeMax: time.Unix(0, 0),
NumTraces: 100,
NumSpans: 10000,
Tags: make(map[string]string),
},
traceIDs: []model.TraceID{
Expand Down
10 changes: 5 additions & 5 deletions plugin/storage/es/spanstore/reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ const (

defaultDocCount = 10000 // the default elasticsearch allowed limit
defaultNumTraces = 100
defaultNumSpans = 10000
Copy link
Member

Choose a reason for hiding this comment

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

this has no effect currently since multi-read is already bound by defaultDocCount

)

var (
Expand Down Expand Up @@ -136,7 +137,7 @@ func (s *SpanReader) GetTrace(ctx context.Context, traceID model.TraceID) (*mode
span, ctx := opentracing.StartSpanFromContext(ctx, "GetTrace")
defer span.Finish()
currentTime := time.Now()
traces, err := s.multiRead(ctx, []string{traceID.String()}, currentTime.Add(-s.maxSpanAge), currentTime)
traces, err := s.multiRead(ctx, []string{traceID.String()}, currentTime.Add(-s.maxSpanAge), currentTime, defaultNumSpans)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -231,15 +232,14 @@ func (s *SpanReader) FindTraces(ctx context.Context, traceQuery *spanstore.Trace
if err != nil {
return nil, err
}
return s.multiRead(ctx, uniqueTraceIDs, traceQuery.StartTimeMin, traceQuery.StartTimeMax)
return s.multiRead(ctx, uniqueTraceIDs, traceQuery.StartTimeMin, traceQuery.StartTimeMax, traceQuery.NumSpans)
}

func (s *SpanReader) FindTraceIDs(ctx context.Context, traceQuery *spanstore.TraceQueryParameters) ([]model.TraceID, error) {
return nil, errors.New("not implemented") // TODO: Implement
}

func (s *SpanReader) multiRead(ctx context.Context, traceIDs []string, startTime, endTime time.Time) ([]*model.Trace, error) {

func (s *SpanReader) multiRead(ctx context.Context, traceIDs []string, startTime, endTime time.Time, numSpans int) ([]*model.Trace, error) {
childSpan, _ := opentracing.StartSpanFromContext(ctx, "multiRead")
childSpan.LogFields(otlog.Object("trace_ids", traceIDs))
defer childSpan.Finish()
Expand Down Expand Up @@ -269,7 +269,7 @@ func (s *SpanReader) multiRead(ctx context.Context, traceIDs []string, startTime
if val, ok := searchAfterTime[traceID]; ok {
nextTime = val
}
searchRequests[i] = elastic.NewSearchRequest().IgnoreUnavailable(true).Type(spanType).Source(elastic.NewSearchSource().Query(query).Size(defaultDocCount).Sort("startTime", true).SearchAfter(nextTime))
searchRequests[i] = elastic.NewSearchRequest().IgnoreUnavailable(true).Type(spanType).Source(elastic.NewSearchSource().Query(query).Size(defaultDocCount).TerminateAfter(numSpans).Sort("startTime", true).SearchAfter(nextTime))
Copy link
Member

Choose a reason for hiding this comment

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

what is the difference between Size() and TerminateAfter()?

}
// set traceIDs to empty
traceIDs = nil
Expand Down
1 change: 1 addition & 0 deletions storage/spanstore/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,5 @@ type TraceQueryParameters struct {
DurationMin time.Duration
DurationMax time.Duration
NumTraces int
NumSpans int
Copy link
Member

Choose a reason for hiding this comment

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

however, why does this need to be passed from the interface? I think the overall feature is intended to protect storage from OOM, so the parameterization should be done via a parameter when instantiating the storage, not at query time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @yurishkuro, I think this particular issue was filed for OOM on the jaeger-query nodes, and we have a separate issue for handling overloading of the underlying storage - #960

Copy link
Member

Choose a reason for hiding this comment

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

Fixing it in the storage will address query-service OOM.

Copy link
Member Author

@annanay25 annanay25 Jan 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

OK. Will look into. Closing this PR.

So do we want to restrict number of spans (per trace) at ingestion? @yurishkuro

Copy link
Member

Choose a reason for hiding this comment

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

Not at ingestion (which isn't possible anyway since spans arrive async). We want the limit to be applied at query time, similar to the changes you made to plugin/storage/es/spanstore/reader.go, but the value of the limit to be configurable as a parameter of the query-service, passed at Reader construction from CLI, not as a parameter of the HTTP request.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry its taking so many iterations to get this one right. Understood now, sending in a new, clean PR.

}