[WIP][jaeger-v2][storage] Implement read path for v2 storage interface #6170
Conversation
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6170 +/- ##
==========================================
- Coverage 96.47% 96.45% -0.03%
==========================================
Files 354 355 +1
Lines 20126 20159 +33
==========================================
+ Hits 19417 19444 +27
- Misses 524 528 +4
- Partials 185 187 +2
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 <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
mahadzaryab1
added
changelog:bugfix-or-minor-feature
changelog:new-feature
storage_v2/spanstore/factory.go
Outdated
| @@ -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?
There was a problem hiding this comment.
no, I think we should keep that same interface for now. Dependencies are not part of OTLP, so we always need our own model, which we have in v1 interface, so I don't see a strong reason to create v2 of it.
There was a problem hiding this comment.
sounds good! i changed the storage extension to initialize v1 and v2 factories
cmd/jaeger/internal/extension/jaegerstorage/factoryadapter/reader.go
Outdated
Show resolved
Hide resolved
| 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).
There was a problem hiding this comment.
Do we want to take the traceReader as part of the constructor, hold it in the QueryService struct and then perform the conversion before calling the underlying method
yes
Do we want to use the storage v2 factory in v1 context as well?
Not quite following, what do you mean by context?
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test