Upgrade storage integration test to v2 Trace Reader #6388
Conversation
ekefan
force-pushed
the
upgrage-storage-integration
branch
3 times, most recently
from
7b5ae83 to
b3a2eb1
Compare
|
Hello @yurishkuro, please review this implementation. |
|
I need to go through it more carefully but on a brief scan looks very much in the right direction. |
yurishkuro
added
the
changelog:ci
ekefan
force-pushed
the
upgrage-storage-integration
branch
from
b3a2eb1 to
3908c5a
Compare
ekefan
force-pushed
the
upgrage-storage-integration
branch
3 times, most recently
from
7ccf5f7 to
1760562
Compare
|
sorry, I don't follow what recent changes are you making, the code looks the same as before. |
Apologies, I haven't been able to make changes yet. I made a mistake trying to update this branch with |
|
make sure to hit Update Branch button before continuing. |
ekefan
force-pushed
the
upgrage-storage-integration
branch
from
1760562 to
ad46425
Compare
`StorageIntegrationV2` supports v2 storage api interfaces as StorageIntegration still supports v1 api Provide a function for extracting flat array of spans from otel traces Signed-off-by: Emmanuel Emonueje Ebenezer <[email protected]>
Accept combination of change in the file Signed-off-by: Emmanuel Emonueje Ebenezer <[email protected]>
…ckend Signed-off-by: Ebenezer <[email protected]> Signed-off-by: Emmanuel Emonueje Ebenezer <[email protected]>
Signed-off-by: Emmanuel Emonueje Ebenezer <[email protected]>
…2ToModel` Signed-off-by: Emmanuel Emonueje Ebenezer <[email protected]>
Upgraded to TraceReader for v2 storage api Signed-off-by: Emmanuel Emonueje Ebenezer <[email protected]>
Signed-off-by: Emmanuel Emonueje Ebenezer <[email protected]>
Signed-off-by: Emmanuel Emonueje Ebenezer <[email protected]>
ekefan
force-pushed
the
upgrage-storage-integration
branch
from
ad46425 to
aa18bc4
Compare
storage_v2/v1adapter/ptrace2model.go
Outdated
| span.Process = &model.Process{} | ||
| span.Process.ServiceName = batch.GetProcess().GetServiceName() | ||
| span.Process.Tags = batch.GetProcess().GetTags() |
There was a problem hiding this comment.
what we usually do in this case is
if span.Process == nil {
span.Process = batch.Process
}
There was a problem hiding this comment.
Alright...
I tried updating the span process everytime, regardless, but the comparator would pick up a difference I couldn't notice by looking at json data. But I'll try with this.
There was a problem hiding this comment.
reusing the Process from Batch avoids unnecessary memory allocations.
There was a problem hiding this comment.
Okay,
I tried again though, I don't know why yet, but for badger storage (the test I tried) it does this only for those cases.
There was a problem hiding this comment.
I spent an hour debugging but couldn't find the root cause. I even wrote a simple test that does a roundtrip from model -> ptrace -> model where we can see that if Process.Tags==[] at the start it becomes nil after roundtrip. However, pretty.Diff did not complain about that, so I am not sure why it was complaining in the badger test (it complained exactly about that, that actual Process.Tags == nil while expected process had [].
Signed-off-by: Emmanuel Emonueje Ebenezer <[email protected]>
yurishkuro
changed the title
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6388 +/- ##
==========================================
- Coverage 96.26% 96.25% -0.01%
==========================================
Files 368 368
Lines 20979 21008 +29
==========================================
+ Hits 20196 20222 +26
- Misses 599 601 +2
- Partials 184 185 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
great start @ekefan ! I changed the description of PR a bit because it does not fully resolve the original issue. |
Which problem is this PR solving?
Description of the changes
StorageIntegrationto align with v2 storage api while supporting v1 apiHow was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test