[v2][storage] Move span reader decorator to storage factories #6280

mahadzaryab1 · 2024-11-29T20:21:29Z

Which problem is this PR solving?

Towards How to capture storage metrics #6219

Description of the changes

This PR moves the decoration of the span readers with the metrics factory to inside the storage factory itself rather than handling it a higher level (ex. all-in-one, query server/extension).
For v2, the namespacing of the metrics has been moved from the query extension to the storage extension.
For v1, the namespacing of the metrics has been moved from the various binaries to the storage meta-factory.
This PR contains a few breaking changes as it changes the namespace under which the following metrics are published:
- Storage specific metrics were that were being pushed under jaeger_query to will now be pushed undder jaeger_storage
- Cassandra specific metrics were pushed under jaeger_cassandra and jaeger_cassandra-archive will now be pushed under jaeger_storage with the following tags:
  - kind=cassandra
  - role=primary or role=archive
  - name with the name of the storage (in jaeger-v2 only)
- ElasticSearch/OpenSearch specific metrics were being pushed under jaeger_index_create will now be pushed under jaeger_storage_index_create with the following tags:
  - kind=elasticsearch/kind=opensearch
  - role=primary or role=archive
  - name with the name of the storage (in jaeger-v2 only)

How was this change tested?

CI
We've booked Implement framework to validate backwards compatibility of metrics #6278 to implement a framework that will allow for validation of metrics

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 · 2024-11-29T20:27:21Z

@yurishkuro The decorator is also used for the metrics reader (https://github.com/jaegertracing/jaeger/blob/main/cmd/jaeger/internal/extension/jaegerquery/server.go#L165). Do you have any thoughts on how we should handle this? This one is a bit different because the underlying MetricsFactory does not accept metrics.Factory at the moment.

codecov · 2024-11-29T20:29:09Z

Codecov Report

Attention: Patch coverage is 93.70629% with 9 lines in your changes missing coverage. Please review.

Project coverage is 96.22%. Comparing base (e9fac05) to head (d0319f1).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
plugin/storage/cassandra/factory.go	81.81%	4 Missing and 2 partials ⚠️
plugin/storage/es/factory.go	90.32%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6280      +/-   ##
==========================================
+ Coverage   96.20%   96.22%   +0.01%     
==========================================
  Files         356      356              
  Lines       20303    20416     +113     
==========================================
+ Hits        19533    19645     +112     
  Misses        583      583              
- Partials      187      188       +1

Flag	Coverage Δ
badger_v1	`8.90% <2.80%> (+0.78%)`	⬆️
badger_v2	`1.63% <0.00%> (-0.02%)`	⬇️
cassandra-4.x-v1-manual	`14.84% <25.23%> (+0.55%)`	⬆️
cassandra-4.x-v2-auto	`1.57% <0.00%> (-0.02%)`	⬇️
cassandra-4.x-v2-manual	`1.57% <0.00%> (-0.02%)`	⬇️
cassandra-5.x-v1-manual	`14.84% <25.23%> (+0.55%)`	⬆️
cassandra-5.x-v2-auto	`1.57% <0.00%> (-0.02%)`	⬇️
cassandra-5.x-v2-manual	`1.57% <0.00%> (-0.02%)`	⬇️
elasticsearch-6.x-v1	`18.59% <19.62%> (+0.45%)`	⬆️
elasticsearch-7.x-v1	`18.68% <19.62%> (+0.45%)`	⬆️
elasticsearch-8.x-v1	`18.84% <19.62%> (+0.44%)`	⬆️
elasticsearch-8.x-v2	`1.63% <0.00%> (-0.01%)`	⬇️
grpc_v1	`10.37% <36.44%> (+1.09%)`	⬆️
grpc_v2	`7.90% <16.08%> (+0.95%)`	⬆️
kafka-v1	`8.58% <0.00%> (-0.08%)`	⬇️
kafka-v2	`1.63% <0.00%> (-0.02%)`	⬇️
memory_v2	`1.63% <0.00%> (-0.02%)`	⬇️
opensearch-1.x-v1	`18.72% <19.62%> (+0.44%)`	⬆️
opensearch-2.x-v1	`18.73% <19.62%> (+0.45%)`	⬆️
opensearch-2.x-v2	`1.62% <0.00%> (-0.03%)`	⬇️
tailsampling-processor	`0.45% <0.00%> (-0.01%)`	⬇️
unittests	`95.13% <88.11%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cmd/jaeger/internal/extension/jaegerstorage/extension.go

yurishkuro · 2024-11-29T20:38:52Z

plugin/storage/factory.go

+				Name: "storage",
+				Tags: map[string]string{
+					"name": name,
+					// TODO: how should we get kind?


for name, factory is actually for kind, factory, because in v1 there is notion of storage names. The closest v1 has to names is primary/archive, which you might add to the namespace as a name attribute in CreateReader vs. CreateArchiveReader

@yurishkuro We're not recording any metrics today for the archive span reader so can we just leave the name tag off?

plugin/storage/factory.go

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 · 2024-11-29T21:21:01Z

@yurishkuro do we need to decorate the span reader in blackhole storage?

yurishkuro · 2024-11-29T21:25:57Z

plugin/storage/cassandra/factory.go

+	if err != nil {
+		return sr, err
+	}
+	return spanstoremetrics.NewReaderDecorator(sr, f.metricsFactory), nil


wrap CreateArchiveSpanReader to? This is where you can apply a label name=primary|archive before creating decorator

@yurishkuro would we need to namespace the metric factory again? The factory doesn't expose a way to just label an existing namespace.

why not - you can pass just the tags, with empty name

oh okay got it - but the name label is already provided for v2 and we don't want to override it. Should we give the label a different name? What about type?

maybe role then

type and kind are kind of the same.

@yurishkuro Sounds good. One other thing to note about cassandra is that it currently holds two metrics factories that publish metrics under jaeger_cassandra_*** and jaeger_cassandra-archive_*** (https://github.com/jaegertracing/jaeger/blob/main/plugin/storage/cassandra/factory.go#L136-L137). Do we want to change these namespaces to match the same ones we're using for the decorator? So we could publish under jaeger_storage_*** with the role, kind, and name (only for v2) labels instead of jaeger_cassandra_*** and jaeger_cassandra-archive_*** .

yes, I was thinking that too, since we're making a breaking change anyway let's make them more consistent.

@yurishkuro Made this change and updated the PR with a summary of the breaking changes

yurishkuro · 2024-11-29T21:28:31Z

do we need to decorate the span reader in blackhole storage?

no, it's not used in prod.

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro · 2024-11-30T16:00:40Z

plugin/storage/cassandra/factory.go

@@ -133,8 +133,7 @@ func (f *Factory) configureFromOptions(o *Options) {

 // Initialize implements storage.Factory
 func (f *Factory) Initialize(metricsFactory metrics.Factory, logger *zap.Logger) error {
-	f.primaryMetricsFactory = metricsFactory.Namespace(metrics.NSOptions{Name: "cassandra", Tags: nil})
-	f.archiveMetricsFactory = metricsFactory.Namespace(metrics.NSOptions{Name: "cassandra-archive", Tags: nil})


Why not keep these two here? Otherwise you're duplicating namespace assignments twice, which means they can get out of sync.

@yurishkuro Done. However, CreateSamplingStore (https://github.com/jaegertracing/jaeger/blob/main/plugin/storage/cassandra/factory.go#L211) and CreateDependencyReader (https://github.com/jaegertracing/jaeger/blob/main/plugin/storage/cassandra/factory.go#L175) will now have the role=primary attached to the metrics they emit. Is this fine? or should they just be emitting metrics under the namespace passed into the storage factory without the role tag?

I think sampling store should be kind=cassandra, role=sampling

@yurishkuro Done. What about the dependency reader? I'm using the base factory for now.

what would it look like? Dependencies technically were always bundled within the spanstore, not a different "role".

the metrics would be published under jaeger_storage_*** with kind=cassandra but no role tag.

that's fine

although since it's using the primary connection / session I would pass it the primary metrics

plugin/storage/grpc/factory_test.go

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 added 8 commits November 29, 2024 13:28

Add Storage Metrics To Memory Span Reader

a2ee10e

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Add Storage Metrics To Badger Span Reader

a156666

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Add Storage Metrics To Cassandra Span Reader

3584e8c

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Add Storage Metrics To ElasticSearch Span Reader

b25b893

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Add Storage Metrics To GRPC Span Reader

d7d2bc9

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Remove Decorator From Query Extension

b63af21

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Namespace Metrics Factory During Initialization In Meta Factory

294c5d0

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Remove Span Reader Decorator In All In One And Query

f8192a0

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 added the changelog:breaking-change Change that is breaking public APIs or established behavior label Nov 29, 2024

yurishkuro reviewed Nov 29, 2024

View reviewed changes

cmd/jaeger/internal/extension/jaegerstorage/extension.go Outdated Show resolved Hide resolved

yurishkuro reviewed Nov 29, 2024

View reviewed changes

plugin/storage/factory.go Outdated Show resolved Hide resolved

yurishkuro reviewed Nov 29, 2024

View reviewed changes

plugin/storage/factory.go Outdated Show resolved Hide resolved

Address Feedback From PR Review

6f678c5

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 marked this pull request as ready for review November 29, 2024 21:17

mahadzaryab1 requested a review from a team as a code owner November 29, 2024 21:17

mahadzaryab1 requested a review from pavolloffay November 29, 2024 21:17

dosubot bot added area/storage storage/badger Issues related to badger storage storage/cassandra storage/elasticsearch v2 labels Nov 29, 2024

mahadzaryab1 changed the title ~~[WIP][v2][storage] Move span reader decorator to storage factories~~ [v2][storage] Move span reader decorator to storage factories Nov 29, 2024

yurishkuro reviewed Nov 29, 2024

View reviewed changes

mahadzaryab1 added 2 commits November 29, 2024 22:48

Standardize Primary And Archive Metrics In Cassandra Factory

6ac1aea

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Standardize Metrics In ES Storage

6744399

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 added 3 commits November 29, 2024 22:56

Standardize Metrics In Memory Storage

0fd8360

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Standardize Metrics In GRPC Storage

f7547da

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Remove Unused Metrics Factory From ES Reader

214aac0

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 requested a review from yurishkuro November 30, 2024 04:08

yurishkuro reviewed Nov 30, 2024

View reviewed changes

plugin/storage/grpc/factory_test.go Show resolved Hide resolved

mahadzaryab1 added 3 commits November 30, 2024 11:11

Move Metrics Assignments To Initialize

e6e8704

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Move Metrics Assignments To Initialize

83aa823

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Fix Test Assertions

46ab305

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 requested a review from yurishkuro November 30, 2024 16:24

mahadzaryab1 added 3 commits November 30, 2024 13:45

Use Different Factory For Sampling

343c90d

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Use Base Factory For Dep Reader

b9b5972

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Use Primary Factory For Dep Reader

a5a673f

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro approved these changes Nov 30, 2024

View reviewed changes

Merge branch 'main' into storage-factory-metrics

6d9e80b

yurishkuro enabled auto-merge (squash) November 30, 2024 19:11

Merge branch 'main' into storage-factory-metrics

d0319f1

yurishkuro disabled auto-merge November 30, 2024 20:12

yurishkuro merged commit 557f149 into jaegertracing:main Nov 30, 2024
53 of 54 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v2][storage] Move span reader decorator to storage factories #6280

[v2][storage] Move span reader decorator to storage factories #6280

mahadzaryab1 commented Nov 29, 2024 •

edited

Loading

mahadzaryab1 commented Nov 29, 2024

codecov bot commented Nov 29, 2024 •

edited

Loading

yurishkuro Nov 29, 2024

mahadzaryab1 Nov 29, 2024

mahadzaryab1 commented Nov 29, 2024

yurishkuro Nov 29, 2024

mahadzaryab1 Nov 29, 2024

yurishkuro Nov 29, 2024

mahadzaryab1 Nov 29, 2024 •

edited

Loading

yurishkuro Nov 29, 2024

yurishkuro Nov 29, 2024

mahadzaryab1 Nov 29, 2024 •

edited

Loading

yurishkuro Nov 29, 2024

mahadzaryab1 Nov 30, 2024

yurishkuro commented Nov 29, 2024

yurishkuro Nov 30, 2024

mahadzaryab1 Nov 30, 2024 •

edited

Loading

yurishkuro Nov 30, 2024

mahadzaryab1 Nov 30, 2024

yurishkuro Nov 30, 2024

mahadzaryab1 Nov 30, 2024

yurishkuro Nov 30, 2024

yurishkuro Nov 30, 2024

mahadzaryab1 Nov 30, 2024

[v2][storage] Move span reader decorator to storage factories #6280

[v2][storage] Move span reader decorator to storage factories #6280

Conversation

mahadzaryab1 commented Nov 29, 2024 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

mahadzaryab1 commented Nov 29, 2024

codecov bot commented Nov 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahadzaryab1 commented Nov 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahadzaryab1 Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahadzaryab1 Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Nov 29, 2024

Choose a reason for hiding this comment

mahadzaryab1 Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahadzaryab1 commented Nov 29, 2024 •

edited

Loading

codecov bot commented Nov 29, 2024 •

edited

Loading

mahadzaryab1 Nov 29, 2024 •

edited

Loading

mahadzaryab1 Nov 29, 2024 •

edited

Loading

mahadzaryab1 Nov 30, 2024 •

edited

Loading