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

Jump to bottom

Fixes Nested Tags Search Error #4651

Closed
wants to merge 1 commit into from
Closed

Fixes Nested Tags Search Error #4651

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions plugin/storage/es/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ func createSpanReader(
SpanIndexRolloverFrequency: cfg.GetIndexRolloverFrequencySpansDuration(),
ServiceIndexRolloverFrequency: cfg.GetIndexRolloverFrequencyServicesDuration(),
TagDotReplacement: cfg.Tags.DotReplacement,
AllTagsAsFields: cfg.Tags.AllAsFields,
UseReadWriteAliases: cfg.UseReadWriteAliases,
Archive: archive,
RemoteReadClusters: cfg.RemoteReadClusters,
Expand Down
15 changes: 11 additions & 4 deletions plugin/storage/es/spanstore/reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ var (

objectTagFieldList = []string{objectTagsField, objectProcessTagsField}

nestedTagFieldList = []string{nestedTagsField, nestedProcessTagsField, nestedLogFieldsField}
nestedTagFieldList = []string{nestedTagsField, nestedProcessTagsField}
)

// SpanReader can query for and load traces from ElasticSearch
Expand All @@ -104,6 +104,7 @@ type SpanReader struct {
spanIndexRolloverFrequency time.Duration
serviceIndexRolloverFrequency time.Duration
spanConverter dbmodel.ToDomain
allTagsAsFields bool
timeRangeIndices timeRangeIndexFn
sourceFn sourceFn
maxDocCount int
Expand All @@ -123,6 +124,7 @@ type SpanReaderParams struct {
SpanIndexRolloverFrequency time.Duration
ServiceIndexRolloverFrequency time.Duration
TagDotReplacement string
AllTagsAsFields bool
Archive bool
UseReadWriteAliases bool
RemoteReadClusters []string
Expand Down Expand Up @@ -150,6 +152,7 @@ func NewSpanReader(p SpanReaderParams) *SpanReader {
spanIndexRolloverFrequency: p.SpanIndexRolloverFrequency,
serviceIndexRolloverFrequency: p.SpanIndexRolloverFrequency,
spanConverter: dbmodel.NewToDomain(p.TagDotReplacement),
allTagsAsFields: p.AllTagsAsFields,
timeRangeIndices: getTimeRangeIndexFn(p.Archive, p.UseReadWriteAliases, p.RemoteReadClusters),
sourceFn: getSourceFn(p.Archive, p.MaxDocCount),
maxDocCount: p.MaxDocCount,
Expand Down Expand Up @@ -668,14 +671,18 @@ func (s *SpanReader) buildOperationNameQuery(operationName string) elastic.Query

func (s *SpanReader) buildTagQuery(k string, v string) elastic.Query {
objectTagListLen := len(objectTagFieldList)
queries := make([]elastic.Query, len(nestedTagFieldList)+objectTagListLen)
queries := make([]elastic.Query, objectTagListLen)
kd := s.spanConverter.ReplaceDot(k)
for i := range objectTagFieldList {
queries[i] = s.buildObjectQuery(objectTagFieldList[i], kd, v)
}
for i := range nestedTagFieldList {
queries[i+objectTagListLen] = s.buildNestedQuery(nestedTagFieldList[i], k, v)
if !s.allTagsAsFields {
for i := range nestedTagFieldList {
queries = append(queries, s.buildNestedQuery(nestedTagFieldList[i], k, v))
}
}
// Nested Logs Query
Copy link
Member

Choose a reason for hiding this comment

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

explain why this is separate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we had all the nested fields query together initally (tags , process tags , logs) . When we set AllTagsAsFields as true , we still want to query the nested logs field.

Copy link
Member

Choose a reason for hiding this comment

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

is it because with AllTagsAsFields=true the tags from logs are still written as nested? Then how does this fix prevent the reported issue? When a span has no logs, it has no tags:[] in the document, but the query would still be looking for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, when AllTagsAsFields is true, logs are written as nested field. Arent logs and tags two independent fields in span context ? The change that I made does not search in the tags field and the process.tags field when allTagsAsFields is true. The reported issue said that they had a different mapping configuration than the jaeger default configuration and hence it never populated the tags field.

Copy link
Member

Choose a reason for hiding this comment

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

that's my point - we should have a test that reproduces the reported issue in the first place (maybe as a new integration test), before we try fixing it. Otherwise how do you know that these changes are fixing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I understand that. Its however redundant to query the tags field when all tags as fields is true. I will work on the integration test!

queries = append(queries, s.buildNestedQuery(nestedLogFieldsField, k, v))

// but configuration can change over time
return elastic.NewBoolQuery().Should(queries...)
Expand Down
1 change: 1 addition & 0 deletions plugin/storage/integration/elasticsearch_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ func (s *ESStorageIntegration) initSpanstore(allTagsAsFields, archive bool) erro
IndexPrefix: indexPrefix,
MaxSpanAge: maxSpanAge,
TagDotReplacement: tagKeyDeDotChar,
AllTagsAsFields: allTagsAsFields,
Copy link
Member

Choose a reason for hiding this comment

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

can there be a test that validates this new behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The integration tests were strong and gave the expected behaviour. I can add the unit tests.

Copy link
Member

Choose a reason for hiding this comment

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

but those tests didn't catch the issue reported by user?

Archive: archive,
MaxDocCount: defaultMaxDocCount,
Tracer: tracer.Tracer("test"),
Expand Down