[metrics][storage] Move metrics reader decorator to metrics storage factory #6287
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]>
mahadzaryab1
added
the
changelog:breaking-change
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6287 +/- ##
==========================================
+ Coverage 96.18% 96.21% +0.02%
==========================================
Files 356 356
Lines 20416 20432 +16
==========================================
+ Hits 19637 19658 +21
+ Misses 589 585 -4
+ Partials 190 189 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -9,6 +9,7 @@ import ( | |||
| "github.com/spf13/viper" | |||
There was a problem hiding this comment.
plugin/metrics is a bad name, we should rename it to plugin/metricstore
There was a problem hiding this comment.
@yurishkuro I can do that in a follow-up PR
There was a problem hiding this comment.
we can also rename storage/metricsstore to storage/metricstore (single s)
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
plugin/metrics/prometheus/factory.go
Outdated
| return prometheusstore.NewMetricsReader(f.options.Configuration, f.logger, f.tracer) | ||
| mr, err := prometheusstore.NewMetricsReader(f.options.Configuration, f.telset.Logger, f.telset.TracerProvider) | ||
| if err != nil { | ||
| return mr, err |
There was a problem hiding this comment.
| return mr, err | |
| return nil, err |
There was a problem hiding this comment.
@yurishkuro We'll need to dip into the implementation of prometheusstore.NewMetricsReader to force an error here. Is that fine? Also, since we're just decorating the reader, do we want to force returning a nil if there is an error? My thinking was that we just pass along whatever it is we get without decorating the reader.
There was a problem hiding this comment.
You can simulate the error easily by passing a TLS config with "foobar" for some of the certificates.
It is convention to return nil, err in case of errors. It's probably what you would get from the factory already, but when you return mr, err you are returning a typed nil, so the nil check may actually fail in the caller.
There was a problem hiding this comment.
@yurishkuro sounds good! i'll also go back and do the same for the other factories in a follow-up PR
Signed-off-by: Mahad Zaryab <[email protected]>
Which problem is this PR solving?
Description of the changes
jaeger_querywill now be published underjaeger_storagewith thekindandrole=metricstoretags.jaeger_metricstorewill now be published underjaeger_storagewith thename,kind, androle=metricstoretags.How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test