[storage][metrics] Implement decorator to wrap storage.Factory #6267
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6267 +/- ##
==========================================
+ Coverage 96.41% 96.44% +0.02%
==========================================
Files 355 356 +1
Lines 20152 20170 +18
==========================================
+ Hits 19430 19453 +23
+ Misses 532 528 -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. |
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
| @@ -60,14 +60,13 @@ func (s *server) Start(ctx context.Context, host component.Host) error { | |||
| if err != nil { | |||
| return fmt.Errorf("cannot find primary storage %s: %w", s.config.Storage.TracesPrimary, err) | |||
| } | |||
| f = storagemetrics.NewDecoratorFactory(f, baseFactory) | |||
There was a problem hiding this comment.
@yurishkuro this is will take in a baseFactory which will have the query namespace and then the decorator will add the query namespace. Just wanted to confirm that this is what we want? Note that the queryMetricsFactory is still being passed in when creating the server.
There was a problem hiding this comment.
we don't want query namespace twice. Perhaps we don't want it at all - why is it query and not storage?
There was a problem hiding this comment.
@yurishkuro It was query before which is why I tried to leave it the same. Do we want to change it to storage given that this is only for v2?
There was a problem hiding this comment.
yes, if we can do that without affecting v1
There was a problem hiding this comment.
@yurishkuro I changed the decorator factory to namespace with storage and left queryMetricsFactory as is. Let me know what you think.
mahadzaryab1
changed the title
| @@ -60,14 +60,13 @@ func (s *server) Start(ctx context.Context, host component.Host) error { | |||
| if err != nil { | |||
| return fmt.Errorf("cannot find primary storage %s: %w", s.config.Storage.TracesPrimary, err) | |||
| } | |||
| f = storagemetrics.NewDecoratorFactory(f, baseFactory) | |||
There was a problem hiding this comment.
Why do you apply the decorator in jaegerquery extension? I would do it in the storage extension, and attach a label storage_kind={name}
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
| func (df *DecoratorFactory) Close() error { | ||
| if closer, ok := df.delegate.(io.Closer); ok { | ||
| return closer.Close() | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
@yurishkuro I moved the decorator to the storage extension but it also requires the Close function. Is it fine to implement it here like this?
There was a problem hiding this comment.
It also looks like some tests are failing because some other interfaces aren't implemented here (sampling store, purger, etc.). Do we want to continue with this approach? It looks like we're losing some important information by wrapping this decorator in the extension
| metrics.NSOptions{ | ||
| Name: "storage", | ||
| Tags: map[string]string{ | ||
| "kind": storageName, |
There was a problem hiding this comment.
"kind" should be something like "cassandra" (which you can save from the switch statement above.
| "kind": storageName, | |
| "name": storageName, | |
| "kind": storageKind, |
| metricsFactory metrics.Factory | ||
| } | ||
|
|
||
| func NewDecoratorFactory(f storage.Factory, mf metrics.Factory) *DecoratorFactory { |
There was a problem hiding this comment.
let's add a comment describing what this does. I would also mention that the metrics namespace is used as is, without enrichment, and it's the caller responsibility to define a desired namespace.
|
Going to take a different approach of decorating the span reader within the storage factories |
Which problem is this PR solving?
Description of the changes
storage.Factoryto add metrics to the functions that need them. This will unblock [v2][storage] Implement read path for v2 storage interface #6170.How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test