Add metrics storage to storage capabilities #5774
Conversation
Signed-off-by: FlamingSaint <raghuramkannan400@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5774 +/- ##
==========================================
- Coverage 96.66% 96.65% -0.02%
==========================================
Files 342 342
Lines 16515 16519 +4
==========================================
+ Hits 15964 15966 +2
- Misses 362 363 +1
- Partials 189 190 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -301,6 +302,7 @@ func TestServerAddMetricsStorage(t *testing.T) { | |||
| { | |||
| name: "Metrics storage unset", | |||
| config: &Config{}, | |||
| qSvcOpts: &querysvc.QueryServiceOptions{}, | |||
There was a problem hiding this comment.
what's the point of defining these as test case parameter if they don't change across tests?
There was a problem hiding this comment.
I modelled this based on the archive storage test.
| @@ -38,13 +39,14 @@ const ( | |||
| type QueryServiceOptions struct { | |||
| ArchiveSpanReader spanstore.Reader | |||
| ArchiveSpanWriter spanstore.Writer | |||
| MetricsReader metricsstore.Reader | |||
There was a problem hiding this comment.
Query service constructor accepts metric reader as argument. Having it also as a field in these options is redundant and confusing to the callers of the constructor
There was a problem hiding this comment.
I don't think the query service constructor accepts metric reader as an argument . It only accepts span reader, dependency reader and options.
There was a problem hiding this comment.
Then it's even worse, if QueryService struct is only designed to serve trace queries, there's absolutely no reason to introduce metrics reader into it. Metrics reader is actually a counterpart for QueryService. It's a poor design to couple them in any way.
Notice that QueryService itself does not serve any remote endpoints, it's purely an internal API. The capabilities struct is served via remote endpoint by app.Server (which does have access to move qs and mqs), and this is where the logic should be, ideally.
| @@ -134,6 +136,7 @@ func (qs QueryService) GetDependencies(ctx context.Context, endTs time.Time, loo | |||
| func (qs QueryService) GetCapabilities() StorageCapabilities { | |||
| return StorageCapabilities{ | |||
| ArchiveStorage: qs.options.hasArchiveStorage(), | |||
| MetricStorage: qs.options.hasMetricStorage(), | |||
There was a problem hiding this comment.
why does it need to interrogate Options if you have access to qs which likely already stores the reference to metric reader?
There was a problem hiding this comment.
No qs doesn't store the reference to the metrics reader. The options does.
Yes, I will investigate if this is needed by the UI. |
Signed-off-by: FlamingSaint <raghuramkannan400@gmail.com>
|
@yurishkuro Closing this PR based on today's discussion. |
Which problem is this PR solving?
Description of the changes
How was this change tested?
make testChecklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test