[v2][storage] Implement read path for v2 storage interface #6170

mahadzaryab1 · 2024-11-06T23:56:12Z

Which problem is this PR solving?

Towards Implement Storage API v2 that operates on OTLP batches #5079

Description of the changes

Implemented the read path for the v2 storage interface. This path currently just wraps a v1 span reader and exposes a static method to access the v1 reader.
Change the jaeger query extension to initialize a v2 storage factory and obtain the v1 span reader from it.
This will unblock the development of more efficient v2 storage implementations, like ClickHouse.

How was this change tested?

Added unit tests for new functionality

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

codecov · 2024-11-07T00:43:15Z

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.21%. Comparing base (557f149) to head (f4f6cfb).

Files with missing lines	Patch %	Lines
...md/jaeger/internal/extension/jaegerquery/server.go	80.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6170      +/-   ##
==========================================
+ Coverage   96.19%   96.21%   +0.01%     
==========================================
  Files         356      356              
  Lines       20416    20436      +20     
==========================================
+ Hits        19640    19662      +22     
+ Misses        587      585       -2     
  Partials      189      189

Flag	Coverage Δ
badger_v1	`8.83% <0.00%> (-0.07%)`	⬇️
badger_v2	`1.63% <0.00%> (-0.01%)`	⬇️
cassandra-4.x-v1-manual	`14.73% <0.00%> (-0.11%)`	⬇️
cassandra-4.x-v2-auto	`1.57% <0.00%> (-0.01%)`	⬇️
cassandra-4.x-v2-manual	`1.57% <0.00%> (-0.01%)`	⬇️
cassandra-5.x-v1-manual	`14.73% <0.00%> (-0.11%)`	⬇️
cassandra-5.x-v2-auto	`1.57% <0.00%> (-0.01%)`	⬇️
cassandra-5.x-v2-manual	`1.57% <0.00%> (-0.01%)`	⬇️
elasticsearch-6.x-v1	`18.46% <0.00%> (-0.14%)`	⬇️
elasticsearch-7.x-v1	`18.54% <0.00%> (-0.15%)`	⬇️
elasticsearch-8.x-v1	`18.71% <0.00%> (-0.14%)`	⬇️
elasticsearch-8.x-v2	`1.62% <0.00%> (-0.01%)`	⬇️
grpc_v1	`10.28% <0.00%> (-0.08%)`	⬇️
grpc_v2	`7.89% <0.00%> (-0.01%)`	⬇️
kafka-v1	`8.52% <0.00%> (-0.07%)`	⬇️
kafka-v2	`1.63% <0.00%> (-0.01%)`	⬇️
memory_v2	`1.63% <0.00%> (-0.01%)`	⬇️
opensearch-1.x-v1	`18.59% <0.00%> (-0.14%)`	⬇️
opensearch-2.x-v1	`18.60% <0.00%> (-0.14%)`	⬇️
opensearch-2.x-v2	`1.62% <0.00%> (-0.01%)`	⬇️
tailsampling-processor	`0.45% <0.00%> (-0.01%)`	⬇️
unittests	`95.12% <94.11%> (+0.01%)`	⬆️

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.

storage_v2/spanstore/factory.go

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

cmd/jaeger/internal/extension/jaegerquery/server.go

mahadzaryab1 · 2024-11-16T17:22:06Z

cmd/jaeger/internal/extension/jaegerquery/server.go

@@ -66,6 +67,7 @@ func (s *server) Start(ctx context.Context, host component.Host) error {
 	}

 	spanReader = storageMetrics.NewReadMetricsDecorator(spanReader, queryMetricsFactory)
+	traceReader := factoryadapter.NewTraceReader(spanReader)


@yurishkuro should we be getting the trace reader via GetStorageFactoryV2? I wasn't sure if that was sufficient because we decorate the spanreader above. Do we want to create a v2 version of that decorator?

yes, now it should be calling GetStorageFactoryV2. And yes about the decorator. Also, we may want to use otel.MeterProvider in the decorator instead of metrics.Factory (we need to gradually start deprecating metrics.Factory - let's book a ticket for that)

@yurishkuro Changed to call GetStorageFactoryV2. For the deprecation, this is what metrics.Factory currently looks like:

// Factory creates new metrics type Factory interface { Counter(metric Options) Counter Timer(metric TimerOptions) Timer Gauge(metric Options) Gauge Histogram(metric HistogramOptions) Histogram // Namespace returns a nested metrics factory. Namespace(scope NSOptions) Factory }

Looking at OTEL's meter, I see it has a counter, gauge, and histogram. I don't see a Timer - what would be the alternative for that? Also, how do we handle the nesting of namespaces?

Are you trying to implement metrics.Factory on top of OTEL MeterProvider? I think it would be great if we could do that, it would minimize our changes across the code base.

Timer doesn't have to be part of the factory API, it can be a util struct done on top of a Histogram - the only difference of timer from Histogram is that its Record() function is strongly typed to Duration.

Name spacing is generally handled by our implementations as a prefix on the metric name, so it would work just the same for OTEL Meter.

@yurishkuro Would you able to clarify what you mean by Are you trying to implement metrics.Factory on top of OTEL MeterProvider? I was trying to see if we could remove metrics.Factory and just replace it with OTEL's Meter Provider. Is there a different approach you were thinking of?

I was wrong, we already have internal/metrics/otelmetrics/factory.go

I guess you were simply asking about v2 version of decorator that wraps Storage V2 API - the decorator can still work off our internal pkg/metrics API.

sounds good - so do we need to book any work here?

we just need not to drop the metrics for storage - this is related to #6219

storage_v2/factoryadapter/reader.go

mahadzaryab1 · 2024-11-17T13:16:53Z

cmd/jaeger/internal/extension/jaegerquery/server.go

 	if err != nil {
 		return fmt.Errorf("cannot create span reader: %w", err)
 	}

-	spanReader = storageMetrics.NewReadMetricsDecorator(spanReader, queryMetricsFactory)
+	// TODO: decorate trace reader with metrics


@yurishkuro How should we do this? The decorator implements spanstore.Reader so if we create a new one that implements spanstore_v2.Reader, it won't work currently since we only ever call the underlying v1 methods. Let me know what you think.

I would like to defer this if possible - what if we use the existing v1 decorator before creating V2 adapter? I.e. I would wrap the span reader as soon as we get it from the v1 factory.

@yurishkuro Do we still want to use GetStorageFactoryV2? If we do, then the tracereader is created from the spanreader there. The other approach would be to just initialize the trace reader here (which is what I was previously doing) using the factoryadapter rather than getting it from the v2 factory. What do you think?

yurishkuro · 2024-11-18T18:30:40Z

storage_v2/factoryadapter/factory.go

@@ -35,8 +35,12 @@ func (f *Factory) Close(_ context.Context) error {
 }

 // CreateTraceReader implements spanstore.Factory.
-func (*Factory) CreateTraceReader() (spanstore.Reader, error) {


can we move this change to another PR and merge it first?

btw this would be a good use case for sapling

@yurishkuro #6221

…reader (#6221) ## Which problem is this PR solving? - Towards #5079 ## Description of the changes - This PR implements the v2 `spanstore.Reader` interface in the factory adapter through the `TraceReader`, which is currently just a wrapper around the v1 `spanstore.Reader`. The `TraceReader` provides a function `GetV1Reader` that exposes the underlying v1 reader which will be used in #6170. ## How was this change tested? - Added unit tests for new functions ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] 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 <[email protected]>

Signed-off-by: Mahad Zaryab <[email protected]>

yurishkuro · 2024-12-01T01:46:21Z

cmd/jaeger/internal/extension/jaegerquery/server.go

@@ -74,17 +74,21 @@ func (s *server) Start(ctx context.Context, host component.Host) error {
 		Namespace(metrics.NSOptions{Name: "jaeger"}).
 		Namespace(metrics.NSOptions{Name: "query"})

-	f, err := jaegerstorage.GetStorageFactory(s.config.Storage.TracesPrimary, host)
+	v1Factory, err := jaegerstorage.GetStorageFactory(s.config.Storage.TracesPrimary, host)


add a comment why v1 is still needed (because of dependencies)

yurishkuro · 2024-12-01T01:50:53Z

cmd/jaeger/internal/extension/jaegerquery/server.go

 	}

-	depReader, err := f.CreateDependencyReader()
+	depReader, err := v1Factory.CreateDependencyReader()


would it make sense to create storage_v2/depstore/ before continuing, so that we don't have to keep this bifurcation?

yurishkuro

a refactoring suggestion:

storage_v/spanstore -> storage_v/tracestore

mahadzaryab1 added changelog:bugfix-or-minor-feature changelog:new-feature Change that should be called out as new feature in CHANGELOG and removed changelog:bugfix-or-minor-feature labels Nov 7, 2024

mahadzaryab1 marked this pull request as ready for review November 7, 2024 01:22

mahadzaryab1 requested a review from a team as a code owner November 7, 2024 01:22

mahadzaryab1 requested a review from albertteoh November 7, 2024 01:22

dosubot bot added area/storage v2 labels Nov 7, 2024

mahadzaryab1 commented Nov 7, 2024

View reviewed changes

storage_v2/spanstore/factory.go Outdated Show resolved Hide resolved

yurishkuro reviewed Nov 7, 2024

View reviewed changes

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

yurishkuro reviewed Nov 7, 2024

View reviewed changes

cmd/jaeger/internal/extension/jaegerquery/server.go Outdated Show resolved Hide resolved

mahadzaryab1 marked this pull request as draft November 10, 2024 20:17

mahadzaryab1 mentioned this pull request Nov 16, 2024

[chore][storage_v2] move factoryadapter to storage_v2 #6216

Merged

4 tasks

mahadzaryab1 force-pushed the v2-reader branch from 377d27f to 2321c76 Compare November 16, 2024 16:39

mahadzaryab1 changed the title ~~[WIP][jaeger-v2][storage] Implement read path for v2 storage interface~~ [jaeger-v2][storage] Implement read path for v2 storage interface Nov 16, 2024

mahadzaryab1 marked this pull request as ready for review November 16, 2024 17:20

mahadzaryab1 commented Nov 16, 2024

View reviewed changes

yurishkuro reviewed Nov 17, 2024

View reviewed changes

storage_v2/factoryadapter/reader.go Outdated Show resolved Hide resolved

mahadzaryab1 commented Nov 17, 2024

View reviewed changes

mahadzaryab1 requested a review from yurishkuro November 17, 2024 13:27

yurishkuro reviewed Nov 18, 2024

View reviewed changes

mahadzaryab1 mentioned this pull request Nov 19, 2024

[storage][v2] Add reader adapter that just exposes the underlying v1 reader #6221

Merged

4 tasks

mahadzaryab1 added 6 commits November 18, 2024 21:24

Add Reader With V2 Interface

c5600ba

Signed-off-by: Mahad Zaryab <[email protected]>

Hold TraceReader In Query Service

23c7700

Signed-off-by: Mahad Zaryab <[email protected]>

Generate Mocks For V2 Storage

2b53471

Signed-off-by: Mahad Zaryab <[email protected]>

Fix Unit Tests

617c30c

Signed-off-by: Mahad Zaryab <[email protected]>

Implement Create Trace Reader For V2 Factory

dcf6357

Signed-off-by: Mahad Zaryab <[email protected]>

Initialize V2 Factories And Use CreateTraceReader

1175e6e

Signed-off-by: Mahad Zaryab <[email protected]>

Address Feedback For Error Variable

1683007

Signed-off-by: Mahad Zaryab <[email protected]>

mahadzaryab1 force-pushed the v2-reader branch from 494848c to 1683007 Compare November 19, 2024 02:24

Merge branch 'main' into v2-reader

840a679

Signed-off-by: Mahad Zaryab <[email protected]>

mahadzaryab1 mentioned this pull request Nov 28, 2024

[storage][metrics] Implement decorator to wrap storage.Factory #6267

Closed

4 tasks

mahadzaryab1 and others added 5 commits November 30, 2024 12:14

Merge branch 'main' into v2-reader

24a53df

Signed-off-by: Mahad Zaryab <[email protected]>

Fix Errors From Merge Commit

df9875a

Signed-off-by: Mahad Zaryab <[email protected]>

Move V2 Factory Initialization After V1

51bffb4

Signed-off-by: Mahad Zaryab <[email protected]>

Add Missing Tests In Query Service

226e3ca

Signed-off-by: Mahad Zaryab <[email protected]>

Merge branch 'main' into v2-reader

f4f6cfb

Signed-off-by: Mahad Zaryab <[email protected]>

mahadzaryab1 requested a review from yurishkuro November 30, 2024 21:17

mahadzaryab1 changed the title ~~[jaeger-v2][storage] Implement read path for v2 storage interface~~ [v2][storage] Implement read path for v2 storage interface Nov 30, 2024

yurishkuro reviewed Dec 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v2][storage] Implement read path for v2 storage interface #6170

[v2][storage] Implement read path for v2 storage interface #6170

mahadzaryab1 commented Nov 6, 2024 •

edited

Loading

codecov bot commented Nov 7, 2024 •

edited

Loading

mahadzaryab1 Nov 16, 2024 •

edited

Loading

yurishkuro Nov 17, 2024

mahadzaryab1 Nov 17, 2024

yurishkuro Nov 18, 2024

mahadzaryab1 Nov 19, 2024

yurishkuro Nov 19, 2024

yurishkuro Nov 19, 2024

mahadzaryab1 Nov 19, 2024

yurishkuro Nov 19, 2024

mahadzaryab1 Nov 17, 2024 •

edited

Loading

yurishkuro Nov 17, 2024

mahadzaryab1 Nov 17, 2024

yurishkuro Nov 18, 2024

yurishkuro Nov 18, 2024 •

edited

Loading

mahadzaryab1 Nov 19, 2024

yurishkuro Dec 1, 2024

yurishkuro Dec 1, 2024

yurishkuro left a comment

[v2][storage] Implement read path for v2 storage interface #6170

Are you sure you want to change the base?

[v2][storage] Implement read path for v2 storage interface #6170

Conversation

mahadzaryab1 commented Nov 6, 2024 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Nov 7, 2024 • edited Loading

Codecov Report

mahadzaryab1 Nov 16, 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

Choose a reason for hiding this comment

mahadzaryab1 Nov 17, 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

yurishkuro Nov 18, 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

yurishkuro left a comment

Choose a reason for hiding this comment

mahadzaryab1 commented Nov 6, 2024 •

edited

Loading

codecov bot commented Nov 7, 2024 •

edited

Loading

mahadzaryab1 Nov 16, 2024 •

edited

Loading

mahadzaryab1 Nov 17, 2024 •

edited

Loading

yurishkuro Nov 18, 2024 •

edited

Loading