How to capture storage metrics

[yurishkuro]

Opening issue to discuss how Jaeger should provide metrics for storage backends.

Option 1: Metrics are provided via decorator

• [+] The metrics are consistent across all storage implementations
• [+] Less work for storage implementation
• [-] Storage-specific metrics are not captured (unless Meter is given to the storage and it creates its own internal metrics)
• [-] It's unclear whose responsibility it is to instantiate the decorator:
  • [+-] Storage Factory could do it internally - easy to forget, but a good composable solution
    • [-] The name spacing of the metrics still need to be decided by the caller, because in jaeger-v2 the same factory can be used for primary and archive storage and those metrics should be distinct
  • [-] Whoever creates reader / writer then wraps them in metrics decorator

Option 2: Metrics are provided by the storage implementation

• [-] No way to guarantee that metrics are consistent across implementations
• [-] Each storage must implement similar logic
• [+] Caller does not need to worry about metrics, only need to pass Meter at initialization
  • [+-] Caller must provide name spacing

[mahadzaryab1]

@yurishkuro If we can have storage specific metrics, does it makes sense to then go with Option 2?

[yurishkuro]

Not necessarily - we can still pass a telset to the factories to do their custom metrics.

Personally I am leaning to option 2 as well but with a caveat that we provide a shared decorator component. Then each storage factory can still produce consistent metrics with decorator, not duplicate the code, and have the ability to generate their own metrics as well.

[mahadzaryab1]

@yurishkuro Sounds good. So is the next step to provide a decorator similar to https://github.com/jaegertracing/jaeger/blob/main/storage/spanstore/metrics/decorator.go#L46 but with the new TraceReader? The current decorator does implement spanstore.Reader (see https://github.com/jaegertracing/jaeger/blob/main/storage/spanstore/metrics/decorator.go#L65-L70). If we create a v2 version of this using traceReader, it won't be used currently if we just extract the spanReader out of the traceReader. Any thoughts on how we want to proceed there?

[yurishkuro]

I don't think v2 decorator is needed at this point, because we don't have v2 native storage implementations, they all go via v1 factories. So the change would be to modify all v1 factories to

• accept telset
• apply v1 decorator when creating v1 reader / writer
• remove usage of decorator from other places. The responsibility of the callers will be create a namespaced metrics.Factory with the name of the storage (e.g. storage=X attribute). We might have a conflict here with primary/archive, so I would try to have the factory then override the same attribute (I expect namespaces work such that latest wins), and then once we do the refactoring to remove archive factory we can delegate the naming responsibility exclusively to the caller.

This way v2 adapter will automatically get underlying objects with metrics.