[es] Materialize span.kind and span.status tags#7272
[es] Materialize span.kind and span.status tags#7272yurishkuro merged 14 commits intojaegertracing:mainfrom
Conversation
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7272 +/- ##
==========================================
- Coverage 96.19% 96.18% -0.02%
==========================================
Files 372 372
Lines 22333 22346 +13
==========================================
+ Hits 21483 21493 +10
- Misses 636 638 +2
- Partials 214 215 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
|
Hi @yurishkuro, the pr is ready for your review |
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
yurishkuro
left a comment
There was a problem hiding this comment.
Overall looks good, but in this form there is no way to make this change backwards compatible. My original proposal for the feature was:
- ESv2 will always materialize the tags, regardless of the feature setting
- But people who already have data should be able to use the feature to enable reading the old data as either materialize or nested tags
This way new users can go with only materialized implementation, but existing users can enable dual reading for a certain time period until the data written in the old format times out.
|
Hi @yurishkuro, I have include the feature gate into reader.go and enable dualLookup for backwards compatible at 6ec473b and 7e33d9e |
|
Hi @yurishkuro, I think that the dualLookUp feature gate is not needed at all. Specifically in buildTagQuery function in this part of code: which is defines as in this method you can actually see that it build query over objectTagFieldList and nestedTagFieldList which are defined correspondingly as followed: So that must means this method always dualLookUp, so we actually don't need the feature anymore? |
|
hm, yes it looks like we're always dual-querying. |
|
I thought we have a flag that enables outgoing JSON logging in ES storage, this would be a good way to confirm the full query. |
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
|
Hi @yurishkuro, I added this logging code here: and this is what I get, I think this do confirms that we are always dual-querying, so no need for this feature any more: |
Signed-off-by: pipiland2612 <nguyen.t.dang.minh@gmail.com>
ee610d3 to
277b8d7
Compare
|
Base on this, I have removed the feature gate, only include the ensureRequiredTag method in v2 |
Which problem is this PR solving?
🛑 Breaking change
span.kindandspan.status(orerror) to top level fields in the ES document.How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test