Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 22 commits into from
Nov 30, 2024
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
a2ee10e
Add Storage Metrics To Memory Span Reader
mahadzaryab1 Nov 29, 2024
a156666
Add Storage Metrics To Badger Span Reader
mahadzaryab1 Nov 29, 2024
3584e8c
Add Storage Metrics To Cassandra Span Reader
mahadzaryab1 Nov 29, 2024
b25b893
Add Storage Metrics To ElasticSearch Span Reader
mahadzaryab1 Nov 29, 2024
d7d2bc9
Add Storage Metrics To GRPC Span Reader
mahadzaryab1 Nov 29, 2024
b63af21
Remove Decorator From Query Extension
mahadzaryab1 Nov 29, 2024
294c5d0
Namespace Metrics Factory During Initialization In Meta Factory
mahadzaryab1 Nov 29, 2024
f8192a0
Remove Span Reader Decorator In All In One And Query
mahadzaryab1 Nov 29, 2024
6f678c5
Address Feedback From PR Review
mahadzaryab1 Nov 29, 2024
6ac1aea
Standardize Primary And Archive Metrics In Cassandra Factory
mahadzaryab1 Nov 30, 2024
6744399
Standardize Metrics In ES Storage
mahadzaryab1 Nov 30, 2024
0fd8360
Standardize Metrics In Memory Storage
mahadzaryab1 Nov 30, 2024
f7547da
Standardize Metrics In GRPC Storage
mahadzaryab1 Nov 30, 2024
214aac0
Remove Unused Metrics Factory From ES Reader
mahadzaryab1 Nov 30, 2024
e6e8704
Move Metrics Assignments To Initialize
mahadzaryab1 Nov 30, 2024
83aa823
Move Metrics Assignments To Initialize
mahadzaryab1 Nov 30, 2024
46ab305
Fix Test Assertions
mahadzaryab1 Nov 30, 2024
343c90d
Use Different Factory For Sampling
mahadzaryab1 Nov 30, 2024
b9b5972
Use Base Factory For Dep Reader
mahadzaryab1 Nov 30, 2024
a5a673f
Use Primary Factory For Dep Reader
mahadzaryab1 Nov 30, 2024
6d9e80b
Merge branch 'main' into storage-factory-metrics
yurishkuro Nov 30, 2024
d0319f1
Merge branch 'main' into storage-factory-metrics
yurishkuro Nov 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions cmd/all-in-one/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ import (
"github.com/jaegertracing/jaeger/storage/dependencystore"
"github.com/jaegertracing/jaeger/storage/metricsstore/metricstoremetrics"
"github.com/jaegertracing/jaeger/storage/spanstore"
"github.com/jaegertracing/jaeger/storage/spanstore/spanstoremetrics"
)

// all-in-one/main is a standalone full-stack jaeger backend, backed by a memory store
Expand Down Expand Up @@ -223,7 +222,6 @@ func startQuery(
tm *tenancy.Manager,
telset telemetry.Settings,
) *queryApp.Server {
spanReader = spanstoremetrics.NewReaderDecorator(spanReader, telset.Metrics)
qs := querysvc.NewQueryService(spanReader, depReader, *queryOpts)

server, err := queryApp.NewServer(context.Background(), qs, metricsQueryService, qOpts, tm, telset)
Expand Down
3 changes: 0 additions & 3 deletions cmd/jaeger/internal/extension/jaegerquery/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (
"github.com/jaegertracing/jaeger/plugin/metrics/disabled"
"github.com/jaegertracing/jaeger/storage/metricsstore"
"github.com/jaegertracing/jaeger/storage/metricsstore/metricstoremetrics"
"github.com/jaegertracing/jaeger/storage/spanstore/spanstoremetrics"
)

var (
Expand Down Expand Up @@ -85,8 +84,6 @@ func (s *server) Start(ctx context.Context, host component.Host) error {
return fmt.Errorf("cannot create span reader: %w", err)
}

spanReader = spanstoremetrics.NewReaderDecorator(spanReader, telset.Metrics)

depReader, err := f.CreateDependencyReader()
if err != nil {
return fmt.Errorf("cannot create dependencies reader: %w", err)
Expand Down
71 changes: 65 additions & 6 deletions cmd/jaeger/internal/extension/jaegerstorage/extension.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,18 +123,77 @@ func (s *storageExt) Start(_ context.Context, host component.Host) error {
var err error = errors.New("empty configuration")
switch {
case cfg.Memory != nil:
factory, err = memory.NewFactoryWithConfig(*cfg.Memory, telset.Metrics, s.telset.Logger), nil
factory, err = memory.NewFactoryWithConfig(
*cfg.Memory,
telset.Metrics.Namespace(metrics.NSOptions{
mahadzaryab1 marked this conversation as resolved.
Show resolved Hide resolved
Name: "storage",
Tags: map[string]string{
"name": storageName,
"kind": "memory",
},
}),
s.telset.Logger,
), nil
case cfg.Badger != nil:
factory, err = badger.NewFactoryWithConfig(*cfg.Badger, telset.Metrics, s.telset.Logger)
factory, err = badger.NewFactoryWithConfig(
*cfg.Badger,
telset.Metrics.Namespace(metrics.NSOptions{
Name: "storage",
Tags: map[string]string{
"name": storageName,
"kind": "badger",
},
}),
s.telset.Logger)
case cfg.GRPC != nil:
grpcTelset := telset
grpcTelset.Metrics = grpcTelset.Metrics.Namespace(
metrics.NSOptions{
Name: "storage",
Tags: map[string]string{
"name": storageName,
"kind": "grpc",
},
},
)
//nolint: contextcheck
factory, err = grpc.NewFactoryWithConfig(*cfg.GRPC, telset)
factory, err = grpc.NewFactoryWithConfig(*cfg.GRPC, grpcTelset)
case cfg.Cassandra != nil:
factory, err = cassandra.NewFactoryWithConfig(*cfg.Cassandra, telset.Metrics, s.telset.Logger)
factory, err = cassandra.NewFactoryWithConfig(
*cfg.Cassandra,
telset.Metrics.Namespace(metrics.NSOptions{
Name: "storage",
Tags: map[string]string{
"name": storageName,
"kind": "cassandra",
},
}),
s.telset.Logger,
)
case cfg.Elasticsearch != nil:
factory, err = es.NewFactoryWithConfig(*cfg.Elasticsearch, telset.Metrics, s.telset.Logger)
factory, err = es.NewFactoryWithConfig(
*cfg.Elasticsearch,
telset.Metrics.Namespace(metrics.NSOptions{
Name: "storage",
Tags: map[string]string{
"name": storageName,
"kind": "elasticsearch",
},
}),
s.telset.Logger,
)
case cfg.Opensearch != nil:
factory, err = es.NewFactoryWithConfig(*cfg.Opensearch, telset.Metrics, s.telset.Logger)
factory, err = es.NewFactoryWithConfig(
*cfg.Opensearch,
telset.Metrics.Namespace(metrics.NSOptions{
Name: "storage",
Tags: map[string]string{
"name": storageName,
"kind": "opensearch",
},
}),
s.telset.Logger,
)
}
if err != nil {
return fmt.Errorf("failed to initialize storage '%s': %w", storageName, err)
Expand Down
2 changes: 0 additions & 2 deletions cmd/query/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import (
"github.com/jaegertracing/jaeger/plugin/storage"
"github.com/jaegertracing/jaeger/ports"
"github.com/jaegertracing/jaeger/storage/metricsstore/metricstoremetrics"
"github.com/jaegertracing/jaeger/storage/spanstore/spanstoremetrics"
)

func main() {
Expand Down Expand Up @@ -95,7 +94,6 @@ func main() {
if err != nil {
logger.Fatal("Failed to create span reader", zap.Error(err))
}
spanReader = spanstoremetrics.NewReaderDecorator(spanReader, metricsFactory)
dependencyReader, err := storageFactory.CreateDependencyReader()
if err != nil {
logger.Fatal("Failed to create dependency reader", zap.Error(err))
Expand Down
14 changes: 9 additions & 5 deletions plugin/storage/badger/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/jaegertracing/jaeger/storage/dependencystore"
"github.com/jaegertracing/jaeger/storage/samplingstore"
"github.com/jaegertracing/jaeger/storage/spanstore"
"github.com/jaegertracing/jaeger/storage/spanstore/spanstoremetrics"
)

const (
Expand All @@ -50,10 +51,11 @@ var ( // interface comformance checks

// Factory implements storage.Factory for Badger backend.
type Factory struct {
Config *Config
store *badger.DB
cache *badgerStore.CacheStore
logger *zap.Logger
Config *Config
store *badger.DB
cache *badgerStore.CacheStore
logger *zap.Logger
metricsFactory metrics.Factory

tmpDir string
maintenanceDone chan bool
Expand Down Expand Up @@ -115,6 +117,7 @@ func (f *Factory) configure(config *Config) {
// Initialize implements storage.Factory
func (f *Factory) Initialize(metricsFactory metrics.Factory, logger *zap.Logger) error {
f.logger = logger
f.metricsFactory = metricsFactory

opts := badger.DefaultOptions("")

Expand Down Expand Up @@ -173,7 +176,8 @@ func initializeDir(path string) {

// CreateSpanReader implements storage.Factory
func (f *Factory) CreateSpanReader() (spanstore.Reader, error) {
return badgerStore.NewTraceReader(f.store, f.cache), nil
tr := badgerStore.NewTraceReader(f.store, f.cache)
return spanstoremetrics.NewReaderDecorator(tr, f.metricsFactory), nil
}

// CreateSpanWriter implements storage.Factory
Expand Down
9 changes: 8 additions & 1 deletion plugin/storage/cassandra/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
"github.com/jaegertracing/jaeger/storage/dependencystore"
"github.com/jaegertracing/jaeger/storage/samplingstore"
"github.com/jaegertracing/jaeger/storage/spanstore"
"github.com/jaegertracing/jaeger/storage/spanstore/spanstoremetrics"
)

const (
Expand All @@ -50,6 +51,7 @@
type Factory struct {
Options *Options

metricsFactory metrics.Factory
primaryMetricsFactory metrics.Factory
archiveMetricsFactory metrics.Factory
logger *zap.Logger
Expand Down Expand Up @@ -133,6 +135,7 @@

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@mahadzaryab1 mahadzaryab1 Nov 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's fine

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

f.logger = logger
Expand All @@ -157,7 +160,11 @@

// CreateSpanReader implements storage.Factory
func (f *Factory) CreateSpanReader() (spanstore.Reader, error) {
return cSpanStore.NewSpanReader(f.primarySession, f.primaryMetricsFactory, f.logger, f.tracer.Tracer("cSpanStore.SpanReader"))
sr, err := cSpanStore.NewSpanReader(f.primarySession, f.primaryMetricsFactory, f.logger, f.tracer.Tracer("cSpanStore.SpanReader"))
if err != nil {
return sr, err
}

Check warning on line 166 in plugin/storage/cassandra/factory.go

View check run for this annotation

Codecov / codecov/patch

plugin/storage/cassandra/factory.go#L165-L166 

Added lines #L165 - L166 were not covered by tests
return spanstoremetrics.NewReaderDecorator(sr, f.metricsFactory), nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@mahadzaryab1 mahadzaryab1 Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe role then

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type and kind are kind of the same.

Copy link
Collaborator Author

@mahadzaryab1 mahadzaryab1 Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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_*** .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// CreateSpanWriter implements storage.Factory
Expand Down
7 changes: 6 additions & 1 deletion plugin/storage/es/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"github.com/jaegertracing/jaeger/storage/dependencystore"
"github.com/jaegertracing/jaeger/storage/samplingstore"
"github.com/jaegertracing/jaeger/storage/spanstore"
"github.com/jaegertracing/jaeger/storage/spanstore/spanstoremetrics"
)

const (
Expand Down Expand Up @@ -180,7 +181,11 @@ func (f *Factory) getArchiveClient() es.Client {

// CreateSpanReader implements storage.Factory
func (f *Factory) CreateSpanReader() (spanstore.Reader, error) {
return createSpanReader(f.getPrimaryClient, f.primaryConfig, false, f.metricsFactory, f.logger, f.tracer)
sr, err := createSpanReader(f.getPrimaryClient, f.primaryConfig, false, f.metricsFactory, f.logger, f.tracer)
if err != nil {
return sr, err
}
return spanstoremetrics.NewReaderDecorator(sr, f.metricsFactory), nil
}

// CreateSpanWriter implements storage.Factory
Expand Down
12 changes: 10 additions & 2 deletions plugin/storage/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,16 @@ func (*Factory) getFactoryOfType(factoryType string) (storage.Factory, error) {
// Initialize implements storage.Factory.
func (f *Factory) Initialize(metricsFactory metrics.Factory, logger *zap.Logger) error {
f.metricsFactory = metricsFactory
for _, factory := range f.factories {
if err := factory.Initialize(metricsFactory, logger); err != nil {
for name, factory := range f.factories {
if err := factory.Initialize(
metricsFactory.Namespace(metrics.NSOptions{
mahadzaryab1 marked this conversation as resolved.
Show resolved Hide resolved
Name: "storage",
mahadzaryab1 marked this conversation as resolved.
Show resolved Hide resolved
Tags: map[string]string{
"name": name,
// TODO: how should we get kind?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
}),
logger); err != nil {
return err
}
}
Expand Down
3 changes: 2 additions & 1 deletion plugin/storage/grpc/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"github.com/jaegertracing/jaeger/storage"
"github.com/jaegertracing/jaeger/storage/dependencystore"
"github.com/jaegertracing/jaeger/storage/spanstore"
"github.com/jaegertracing/jaeger/storage/spanstore/spanstoremetrics"
)

var ( // interface comformance checks
Expand Down Expand Up @@ -167,7 +168,7 @@ func (f *Factory) newRemoteStorage(

// CreateSpanReader implements storage.Factory
func (f *Factory) CreateSpanReader() (spanstore.Reader, error) {
return f.services.Store.SpanReader(), nil
return spanstoremetrics.NewReaderDecorator(f.services.Store.SpanReader(), f.telset.Metrics), nil
}

// CreateSpanWriter implements storage.Factory
Expand Down
3 changes: 1 addition & 2 deletions plugin/storage/grpc/factory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,8 @@ func TestInitFactory(t *testing.T) {
f := makeFactory(t)
f.services.Capabilities = nil

reader, err := f.CreateSpanReader()
_, err := f.CreateSpanReader()
require.NoError(t, err)
assert.Equal(t, f.services.Store.SpanReader(), reader)
mahadzaryab1 marked this conversation as resolved.
Show resolved Hide resolved

writer, err := f.CreateSpanWriter()
require.NoError(t, err)
Expand Down
3 changes: 2 additions & 1 deletion plugin/storage/memory/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/jaegertracing/jaeger/storage/dependencystore"
"github.com/jaegertracing/jaeger/storage/samplingstore"
"github.com/jaegertracing/jaeger/storage/spanstore"
"github.com/jaegertracing/jaeger/storage/spanstore/spanstoremetrics"
)

var ( // interface comformance checks
Expand Down Expand Up @@ -81,7 +82,7 @@ func (f *Factory) Initialize(metricsFactory metrics.Factory, logger *zap.Logger)

// CreateSpanReader implements storage.Factory
func (f *Factory) CreateSpanReader() (spanstore.Reader, error) {
return f.store, nil
return spanstoremetrics.NewReaderDecorator(f.store, f.metricsFactory), nil
}

// CreateSpanWriter implements storage.Factory
Expand Down
3 changes: 1 addition & 2 deletions plugin/storage/memory/factory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,8 @@ func TestMemoryStorageFactory(t *testing.T) {
f := NewFactory()
require.NoError(t, f.Initialize(metrics.NullFactory, zap.NewNop()))
assert.NotNil(t, f.store)
reader, err := f.CreateSpanReader()
_, err := f.CreateSpanReader()
require.NoError(t, err)
assert.Equal(t, f.store, reader)
writer, err := f.CreateSpanWriter()
require.NoError(t, err)
assert.Equal(t, f.store, writer)
Expand Down
Loading