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

[metrics][storage] Move metrics reader decorator to metrics storage factory #6287

Merged
merged 10 commits into from
Dec 1, 2024
Merged

[metrics][storage] Move metrics reader decorator to metrics storage factory #6287

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a5cfa58
Add MetricsFactory To Initialize Function In Interface
mahadzaryab1 Dec 1, 2024
b7c9f59
Generate New Mocks
mahadzaryab1 Dec 1, 2024
449f08a
Implement New Interface In Disabled Factory
mahadzaryab1 Dec 1, 2024
e7eb7cd
Update Callsite To Pass In Metrics Factory
mahadzaryab1 Dec 1, 2024
1144fe4
Merge branch 'main' into metrics-store-metrics
mahadzaryab1 Dec 1, 2024
aa0323d
Change Name of Lambda Helper
mahadzaryab1 Dec 1, 2024
f2f6714
Accept telemetry.Settings Instead of Metrics Provider
mahadzaryab1 Dec 1, 2024
0d0921f
Add Role Label
mahadzaryab1 Dec 1, 2024
4842837
Merge branch 'main' into metrics-store-metrics
mahadzaryab1 Dec 1, 2024
df0de70
Add Test For Error In CreateMetricsReader
mahadzaryab1 Dec 1, 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
8 changes: 3 additions & 5 deletions cmd/all-in-one/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ import (
"github.com/jaegertracing/jaeger/plugin/storage"
"github.com/jaegertracing/jaeger/ports"
"github.com/jaegertracing/jaeger/storage/dependencystore"
"github.com/jaegertracing/jaeger/storage/metricsstore/metricstoremetrics"
"github.com/jaegertracing/jaeger/storage/spanstore"
)

Expand Down Expand Up @@ -118,7 +117,7 @@ by default uses only in-memory database.`,
logger.Fatal("Failed to create dependency reader", zap.Error(err))
}

metricsQueryService, err := createMetricsQueryService(metricsReaderFactory, v, logger, queryMetricsFactory)
metricsQueryService, err := createMetricsQueryService(metricsReaderFactory, v, logger, baseFactory)
if err != nil {
logger.Fatal("Failed to create metrics reader", zap.Error(err))
}
Expand Down Expand Up @@ -241,7 +240,7 @@ func createMetricsQueryService(
logger *zap.Logger,
metricsReaderMetricsFactory metrics.Factory,
) (querysvc.MetricsQueryService, error) {
if err := metricsReaderFactory.Initialize(logger); err != nil {
if err := metricsReaderFactory.Initialize(metricsReaderMetricsFactory, logger); err != nil {
return nil, fmt.Errorf("failed to init metrics reader factory: %w", err)
}

Expand All @@ -252,6 +251,5 @@ func createMetricsQueryService(
return nil, fmt.Errorf("failed to create metrics reader: %w", err)
}

// Decorate the metrics reader with metrics instrumentation.
return metricstoremetrics.NewReaderDecorator(reader, metricsReaderMetricsFactory), nil
return reader, nil
}
7 changes: 1 addition & 6 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 @@ -15,14 +15,12 @@ import (
"github.com/jaegertracing/jaeger/cmd/jaeger/internal/extension/jaegerstorage"
queryApp "github.com/jaegertracing/jaeger/cmd/query/app"
"github.com/jaegertracing/jaeger/cmd/query/app/querysvc"
"github.com/jaegertracing/jaeger/internal/metrics/otelmetrics"
"github.com/jaegertracing/jaeger/pkg/jtracer"
"github.com/jaegertracing/jaeger/pkg/metrics"
"github.com/jaegertracing/jaeger/pkg/telemetry"
"github.com/jaegertracing/jaeger/pkg/tenancy"
"github.com/jaegertracing/jaeger/plugin/metrics/disabled"
"github.com/jaegertracing/jaeger/storage/metricsstore"
"github.com/jaegertracing/jaeger/storage/metricsstore/metricstoremetrics"
)

var (
Expand Down Expand Up @@ -156,10 +154,7 @@ func (s *server) createMetricReader(host component.Host) (metricsstore.Reader, e
return nil, fmt.Errorf("cannot create metrics reader %w", err)
}

// Decorate the metrics reader with metrics instrumentation.
mf := otelmetrics.NewFactory(s.telset.MeterProvider)
mf = mf.Namespace(metrics.NSOptions{Name: "jaeger_metricstore"})
return metricstoremetrics.NewReaderDecorator(metricsReader, mf), nil
return metricsReader, nil
}

func (s *server) Shutdown(ctx context.Context) error {
Expand Down
2 changes: 1 addition & 1 deletion cmd/jaeger/internal/extension/jaegerquery/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ type fakeMetricsFactory struct {
}

// Initialize implements storage.MetricsFactory.
func (fmf fakeMetricsFactory) Initialize(*zap.Logger) error {
func (fmf fakeMetricsFactory) Initialize(metrics.Factory, *zap.Logger) error {
if fmf.name == "need-initialize-error" {
return errors.New("test-error")
}
Expand Down
5 changes: 4 additions & 1 deletion cmd/jaeger/internal/extension/jaegerstorage/extension.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,10 @@ func (s *storageExt) Start(_ context.Context, host component.Host) error {
var metricsFactory storage.MetricsFactory
var err error
if cfg.Prometheus != nil {
metricsFactory, err = prometheus.NewFactoryWithConfig(*cfg.Prometheus, s.telset.Logger)
metricsFactory, err = prometheus.NewFactoryWithConfig(
*cfg.Prometheus,
getMetricsFactory(metricStorageName, "prometheus"),
yurishkuro marked this conversation as resolved.
Show resolved Hide resolved
mahadzaryab1 marked this conversation as resolved.
Show resolved Hide resolved
s.telset.Logger)
}
if err != nil {
return fmt.Errorf("failed to initialize metrics storage '%s': %w", metricStorageName, err)
Expand Down
8 changes: 3 additions & 5 deletions cmd/query/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import (
metricsPlugin "github.com/jaegertracing/jaeger/plugin/metrics"
"github.com/jaegertracing/jaeger/plugin/storage"
"github.com/jaegertracing/jaeger/ports"
"github.com/jaegertracing/jaeger/storage/metricsstore/metricstoremetrics"
)

func main() {
Expand Down Expand Up @@ -99,7 +98,7 @@ func main() {
logger.Fatal("Failed to create dependency reader", zap.Error(err))
}

metricsQueryService, err := createMetricsQueryService(metricsReaderFactory, v, logger, metricsFactory)
metricsQueryService, err := createMetricsQueryService(metricsReaderFactory, v, logger, baseFactory)
if err != nil {
logger.Fatal("Failed to create metrics query service", zap.Error(err))
}
Expand Down Expand Up @@ -169,7 +168,7 @@ func createMetricsQueryService(
logger *zap.Logger,
metricsReaderMetricsFactory metrics.Factory,
) (querysvc.MetricsQueryService, error) {
if err := metricsReaderFactory.Initialize(logger); err != nil {
if err := metricsReaderFactory.Initialize(metricsReaderMetricsFactory, logger); err != nil {
return nil, fmt.Errorf("failed to init metrics reader factory: %w", err)
}

Expand All @@ -180,6 +179,5 @@ func createMetricsQueryService(
return nil, fmt.Errorf("failed to create metrics reader: %w", err)
}

// Decorate the metrics reader with metrics instrumentation.
return metricstoremetrics.NewReaderDecorator(reader, metricsReaderMetricsFactory), nil
return reader, nil
}
3 changes: 2 additions & 1 deletion plugin/metrics/disabled/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/spf13/viper"
Copy link
Member

Choose a reason for hiding this comment

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

plugin/metrics is a bad name, we should rename it to plugin/metricstore

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 I can do that in a follow-up PR

Copy link
Member

Choose a reason for hiding this comment

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

we can also rename storage/metricsstore to storage/metricstore (single s)

"go.uber.org/zap"

"github.com/jaegertracing/jaeger/pkg/metrics"
"github.com/jaegertracing/jaeger/plugin"
"github.com/jaegertracing/jaeger/storage/metricsstore"
)
Expand All @@ -30,7 +31,7 @@ func (*Factory) AddFlags(_ *flag.FlagSet) {}
func (*Factory) InitFromViper(_ *viper.Viper, _ *zap.Logger) {}

// Initialize implements storage.MetricsFactory.
func (*Factory) Initialize(_ *zap.Logger) error {
func (*Factory) Initialize(_ metrics.Factory, _ *zap.Logger) error {
return nil
}

Expand Down
5 changes: 3 additions & 2 deletions plugin/metrics/disabled/factory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,17 @@ import (
"github.com/stretchr/testify/require"
"go.uber.org/zap"

"github.com/jaegertracing/jaeger/pkg/metrics"
"github.com/jaegertracing/jaeger/storage"
)

var _ storage.MetricsFactory = new(Factory)

func TestPrometheusFactory(t *testing.T) {
f := NewFactory()
require.NoError(t, f.Initialize(zap.NewNop()))
require.NoError(t, f.Initialize(metrics.NullFactory, zap.NewNop()))

err := f.Initialize(nil)
err := f.Initialize(metrics.NullFactory, nil)
require.NoError(t, err)

f.AddFlags(nil)
Expand Down
13 changes: 10 additions & 3 deletions plugin/metrics/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/spf13/viper"
"go.uber.org/zap"

"github.com/jaegertracing/jaeger/pkg/metrics"
"github.com/jaegertracing/jaeger/plugin"
"github.com/jaegertracing/jaeger/plugin/metrics/disabled"
"github.com/jaegertracing/jaeger/plugin/metrics/prometheus"
Expand Down Expand Up @@ -63,9 +64,15 @@ func (*Factory) getFactoryOfType(factoryType string) (storage.MetricsFactory, er
}

// Initialize implements storage.MetricsFactory.
func (f *Factory) Initialize(logger *zap.Logger) error {
for _, factory := range f.factories {
factory.Initialize(logger)
func (f *Factory) Initialize(metricsFactory metrics.Factory, logger *zap.Logger) error {
mahadzaryab1 marked this conversation as resolved.
Show resolved Hide resolved
for kind, factory := range f.factories {
mf := metricsFactory.Namespace(metrics.NSOptions{
Name: "storage",
Tags: map[string]string{
"kind": kind,
mahadzaryab1 marked this conversation as resolved.
Show resolved Hide resolved
},
})
factory.Initialize(mf, logger)
}
return nil
}
Expand Down
3 changes: 2 additions & 1 deletion plugin/metrics/factory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/stretchr/testify/require"
"go.uber.org/zap"

"github.com/jaegertracing/jaeger/pkg/metrics"
"github.com/jaegertracing/jaeger/plugin/metrics/disabled"
"github.com/jaegertracing/jaeger/storage"
"github.com/jaegertracing/jaeger/storage/mocks"
Expand Down Expand Up @@ -53,7 +54,7 @@ func TestCreateMetricsReader(t *testing.T) {
require.NoError(t, err)
require.NotNil(t, f)

require.NoError(t, f.Initialize(zap.NewNop()))
require.NoError(t, f.Initialize(metrics.NullFactory, zap.NewNop()))

reader, err := f.CreateMetricsReader()
require.NoError(t, err)
Expand Down
22 changes: 15 additions & 7 deletions plugin/metrics/prometheus/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,22 @@
"go.opentelemetry.io/otel/trace"
"go.uber.org/zap"

"github.com/jaegertracing/jaeger/pkg/metrics"
"github.com/jaegertracing/jaeger/pkg/prometheus/config"
"github.com/jaegertracing/jaeger/plugin"
prometheusstore "github.com/jaegertracing/jaeger/plugin/metrics/prometheus/metricsstore"
"github.com/jaegertracing/jaeger/storage/metricsstore"
"github.com/jaegertracing/jaeger/storage/metricsstore/metricstoremetrics"
)

var _ plugin.Configurable = (*Factory)(nil)

// Factory implements storage.Factory and creates storage components backed by memory store.
type Factory struct {
options *Options
logger *zap.Logger
tracer trace.TracerProvider
options *Options
logger *zap.Logger
tracer trace.TracerProvider
metricsFactory metrics.Factory
}

// NewFactory creates a new Factory.
Expand All @@ -47,18 +50,23 @@
}

// Initialize implements storage.MetricsFactory.
func (f *Factory) Initialize(logger *zap.Logger) error {
f.logger = logger
func (f *Factory) Initialize(metricsFactory metrics.Factory, logger *zap.Logger) error {
f.metricsFactory, f.logger = metricsFactory, logger
return nil
}

// CreateMetricsReader implements storage.MetricsFactory.
func (f *Factory) CreateMetricsReader() (metricsstore.Reader, error) {
return prometheusstore.NewMetricsReader(f.options.Configuration, f.logger, f.tracer)
mr, err := prometheusstore.NewMetricsReader(f.options.Configuration, f.logger, f.tracer)
if err != nil {
return mr, err
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return mr, err
return nil, err

Copy link
Member

Choose a reason for hiding this comment

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

can we add a test?

Copy link
Collaborator Author

@mahadzaryab1 mahadzaryab1 Dec 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@yurishkuro We'll need to dip into the implementation of prometheusstore.NewMetricsReader to force an error here. Is that fine? Also, since we're just decorating the reader, do we want to force returning a nil if there is an error? My thinking was that we just pass along whatever it is we get without decorating the reader.

Copy link
Member

Choose a reason for hiding this comment

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

You can simulate the error easily by passing a TLS config with "foobar" for some of the certificates.

It is convention to return nil, err in case of errors. It's probably what you would get from the factory already, but when you return mr, err you are returning a typed nil, so the nil check may actually fail in the caller.

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 sounds good! i'll also go back and do the same for the other factories in a follow-up PR

}

Check warning on line 63 in plugin/metrics/prometheus/factory.go

View check run for this annotation

Codecov / codecov/patch

plugin/metrics/prometheus/factory.go#L62-L63 

Added lines #L62 - L63 were not covered by tests
return metricstoremetrics.NewReaderDecorator(mr, f.metricsFactory), nil
}

func NewFactoryWithConfig(
cfg config.Configuration,
metricsFactory metrics.Factory,
logger *zap.Logger,
mahadzaryab1 marked this conversation as resolved.
Show resolved Hide resolved
) (*Factory, error) {
if err := cfg.Validate(); err != nil {
Expand All @@ -68,6 +76,6 @@
f.options = &Options{
Configuration: cfg,
}
f.Initialize(logger)
f.Initialize(metricsFactory, logger)
return f, nil
}
7 changes: 4 additions & 3 deletions plugin/metrics/prometheus/factory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"go.uber.org/zap"

"github.com/jaegertracing/jaeger/pkg/config"
"github.com/jaegertracing/jaeger/pkg/metrics"
promCfg "github.com/jaegertracing/jaeger/pkg/prometheus/config"
"github.com/jaegertracing/jaeger/pkg/testutils"
"github.com/jaegertracing/jaeger/storage"
Expand All @@ -22,7 +23,7 @@ var _ storage.MetricsFactory = new(Factory)

func TestPrometheusFactory(t *testing.T) {
f := NewFactory()
require.NoError(t, f.Initialize(zap.NewNop()))
require.NoError(t, f.Initialize(metrics.NullFactory, zap.NewNop()))
assert.NotNil(t, f.logger)

listener, err := net.Listen("tcp", "localhost:")
Expand Down Expand Up @@ -126,15 +127,15 @@ func TestFailedTLSOptions(t *testing.T) {

func TestEmptyFactoryConfig(t *testing.T) {
cfg := promCfg.Configuration{}
_, err := NewFactoryWithConfig(cfg, zap.NewNop())
_, err := NewFactoryWithConfig(cfg, metrics.NullFactory, zap.NewNop())
require.Error(t, err)
}

func TestFactoryConfig(t *testing.T) {
cfg := promCfg.Configuration{
ServerURL: "localhost:1234",
}
_, err := NewFactoryWithConfig(cfg, zap.NewNop())
_, err := NewFactoryWithConfig(cfg, metrics.NullFactory, zap.NewNop())
require.NoError(t, err)
}

Expand Down
2 changes: 1 addition & 1 deletion storage/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ type ArchiveFactory interface {
type MetricsFactory interface {
// Initialize performs internal initialization of the factory, such as opening connections to the backend store.
// It is called after all configuration of the factory itself has been done.
Initialize(logger *zap.Logger) error
Initialize(metricsFactory metrics.Factory, logger *zap.Logger) error

// CreateMetricsReader creates a metricsstore.Reader.
CreateMetricsReader() (metricsstore.Reader, error)
Expand Down
12 changes: 7 additions & 5 deletions storage/mocks/MetricsFactory.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading