Fixes Nested Tags Search Error #4651
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #4651 +/- ##
=======================================
Coverage 97.06% 97.07%
=======================================
Files 301 301
Lines 17878 17884 +6
=======================================
+ Hits 17354 17361 +7
+ Misses 420 419 -1
Partials 104 104
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
Signed-off-by: bugslayer-332 <ayashwanth9503@gmail.com>
pmuls99
force-pushed
the
fixTagsNestedError
branch
from
06e960c
to
db2245e
Compare
|
@yurishkuro , as discussed in #3900 , I have raised this PR. Could you please take a look and give pointers on improvement? |
pmuls99
changed the title
| } | ||
| // Nested Logs Query |
There was a problem hiding this comment.
explain why this is separate?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
| @@ -168,6 +168,7 @@ func (s *ESStorageIntegration) initSpanstore(allTagsAsFields, archive bool) erro | |||
| IndexPrefix: indexPrefix, | |||
| MaxSpanAge: maxSpanAge, | |||
| TagDotReplacement: tagKeyDeDotChar, | |||
| AllTagsAsFields: allTagsAsFields, | |||
There was a problem hiding this comment.
can there be a test that validates this new behavior?
There was a problem hiding this comment.
The integration tests were strong and gave the expected behaviour. I can add the unit tests.
There was a problem hiding this comment.
but those tests didn't catch the issue reported by user?
Which problem is this PR solving?
Description of the changes