[WIP][jaeger-v2][storage] Implement read path for v2 storage interface #6170
Conversation
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6170 +/- ##
==========================================
- Coverage 96.47% 96.23% -0.25%
==========================================
Files 354 355 +1
Lines 20126 20156 +30
==========================================
- Hits 19417 19397 -20
- Misses 524 575 +51
+ Partials 185 184 -1
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: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
mahadzaryab1
added
changelog:bugfix-or-minor-feature
changelog:new-feature
| @@ -17,4 +18,7 @@ type Factory interface { | |||
|
|
|||
| // CreateTraceWriter creates a spanstore.Writer. | |||
| CreateTraceWriter() (Writer, error) | |||
|
|
|||
| // CreateDependencyReader creates a dependencystore.Reader. | |||
| CreateDependencyReader() (dependencystore.Reader, error) | |||
There was a problem hiding this comment.
@yurishkuro not sure if this belongs here since we're in package spanstore - let me know if you have any thoughts on how we should proceed here
There was a problem hiding this comment.
I think it needs to be a different interface
There was a problem hiding this comment.
@yurishkuro Do we want to add an interface like https://github.com/jaegertracing/jaeger/blob/main/storage/dependencystore/interface.go#L1-L22 in storage_v2 to operate on OTLP data and use the same trick to extract the v1 reader?
| tr, ok := reader.(*TraceReader) | ||
| if !ok { | ||
| return nil, ErrV1ReaderNotAvailable | ||
| } | ||
| return tr.spanReader, nil |
There was a problem hiding this comment.
| tr, ok := reader.(*TraceReader) | |
| if !ok { | |
| return nil, ErrV1ReaderNotAvailable | |
| } | |
| return tr.spanReader, nil | |
| if tr, ok := reader.(*TraceReader); ok { | |
| return tr.spanReader, nil | |
| } | |
| return nil, ErrV1ReaderNotAvailable |
| return fmt.Errorf("cannot create trace reader: %w", err) | ||
| } | ||
|
|
||
| spanReader, err := factoryadapter.GetV1Reader(traceReader) |
There was a problem hiding this comment.
the idea was to push this conversion down into query service (as deep as possible), otherwise we can never make it work with v2 API
There was a problem hiding this comment.
@yurishkuro Oh okay I see. So a couple of follow-ups
- Do we want to take the
traceReaderas part of the constructor, hold it in theQueryServicestruct and then perform the conversion before calling the underlying method (in https://github.com/jaegertracing/jaeger/blob/main/cmd/query/app/querysvc/query_service.go#L63 for example)? - Do we want to use the storage v2 factory in v1 context as well? The factory is initialized differently in v1 (example: https://github.com/jaegertracing/jaeger/blob/main/cmd/query/main.go#L42).
Signed-off-by: Mahad Zaryab <[email protected]>
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test