[WIP][v2][storage] Create v2 query service to operate on otlp data model #6343
Conversation
Signed-off-by: Mahad Zaryab <[email protected]>
| // QueryServiceOptions has optional members of QueryService | ||
| type QueryServiceOptionsV2 struct { | ||
| ArchiveTraceReader tracestore.Reader | ||
| ArchiveTraceWriter tracestore.Writer | ||
| Adjuster adjuster.Adjuster | ||
| } |
There was a problem hiding this comment.
@yurishkuro is there any reason this can't just be part of the QueryServiceV2 struct?
There was a problem hiding this comment.
it's a question of now the constructor organized. It has some mandatory arguments and some optional arguments. I don't like the functional option pattern for optional arguments - too much typing overhead with no additional benefits over just passing an Options struct. So the constructor could theoretically be just New(Settings), but that allows required args not to be set. It could be New(...all args...) which creates very long signature with unchecked semantics (you could mistaken main reader and archive reader positions, for example). In v1 case it's New(reader, deps, options) which clearly indicates required / optional args.
| func (opts *QueryServiceOptionsV2) InitArchiveStorage( | ||
| archiveReader tracestore.Reader, | ||
| archiveWriter tracestore.Writer, | ||
| logger *zap.Logger) { | ||
| opts.ArchiveTraceReader = archiveReader | ||
| opts.ArchiveTraceWriter = archiveWriter | ||
| } |
There was a problem hiding this comment.
@yurishkuro how should we handle this in the new query service? should we just make it part of the constructor?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6343 +/- ##
===========================================
- Coverage 96.14% 48.80% -47.35%
===========================================
Files 357 183 -174
Lines 20547 11262 -9285
===========================================
- Hits 19755 5496 -14259
- Misses 605 5319 +4714
- Partials 187 447 +260
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
mahadzaryab1
changed the title
| // GetCapabilities returns the features supported by the query service. | ||
| func (qs QueryServiceV2) GetCapabilities() StorageCapabilities { | ||
| return StorageCapabilities{ | ||
| ArchiveStorage: qs.options.hasArchiveStorage(), | ||
| } |
There was a problem hiding this comment.
I don't think this is being used - can we remove it?
There was a problem hiding this comment.
cmd/query/app/server.go
190: staticHandlerCloser := RegisterStaticHandler(r, telset.Logger, queryOpts, querySvc.GetCapabilities())
| qsvc.options.Adjuster = adjuster.Sequence(StandardAdjusters(defaultMaxClockSkewAdjustV2)...) | ||
| } | ||
| return qsvc | ||
| } |
There was a problem hiding this comment.
try to reuse the existing tests for v1 query service.
| } | ||
|
|
||
| // InitArchiveStorage tries to initialize archive storage reader/writer if storage factory supports them. | ||
| func (opts *QueryServiceOptionsV2) InitArchiveStorage( |
There was a problem hiding this comment.
this would be weird pattern since we're just passing these directly, so might as well do everything in the constructor.
| type QueryServiceV2 struct { | ||
| traceReader tracestore.Reader | ||
| dependencyReader depstore.Reader | ||
| options QueryServiceOptionsV2 |
There was a problem hiding this comment.
the reason why this is a field and not embedded struct is because doing the latter exposes the optional args as public fields on the main struct.
| var errNoArchiveSpanStorageV2 = errors.New("archive span storage was not configured") | ||
|
|
||
| const ( | ||
| defaultMaxClockSkewAdjustV2 = time.Second | ||
| ) |
There was a problem hiding this comment.
rather than redefining I would move them here from query_service.go and reuse the names. We don't have to duplicate what we don't need to.
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test