Skip to content

Commit

Permalink
[jaeger-v2] Refactor Configuration For Query Service (#5998)
Browse files Browse the repository at this point in the history
## Which problem is this PR solving?
- Part of #5996

## Description of the changes
- Reworked the configuration for `jaeger-query` to have more logical
groupings.
- [Migration
document](https://docs.google.com/document/d/1-01QRgMFWbHGXXJqs5cL4kA-P10BfU0TD9Kvp3rEw3M/edit?usp=sharing)

## How was this change tested?
- CI

## 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 <mahadzaryab1@gmail.com>
  • Loading branch information
mahadzaryab1 committed Sep 25, 2024
1 parent 7c6ed87 commit d7f01d1
Show file tree
Hide file tree
Showing 21 changed files with 160 additions and 108 deletions.
8 changes: 5 additions & 3 deletions cmd/jaeger/config-badger.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@ extensions:
http:

jaeger_query:
trace_storage: some_store
trace_storage_archive: another_store
ui_config: ./cmd/jaeger/config-ui.json
storage:
traces: some_store
traces_archive: another_store
ui:
config_file: ./cmd/jaeger/config-ui.json

jaeger_storage:
backends:
Expand Down
8 changes: 5 additions & 3 deletions cmd/jaeger/config-cassandra.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@ extensions:
http:

jaeger_query:
trace_storage: some_storage
trace_storage_archive: another_storage
ui_config: ./cmd/jaeger/config-ui.json
storage:
traces: some_storage
traces_archive: another_storage
ui:
config_file: ./cmd/jaeger/config-ui.json

jaeger_storage:
backends:
Expand Down
8 changes: 5 additions & 3 deletions cmd/jaeger/config-elasticsearch.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@ extensions:
http:

jaeger_query:
trace_storage: some_storage
trace_storage_archive: another_storage
ui_config: ./cmd/jaeger/config-ui.json
storage:
traces: some_storage
traces_archive: another_storage
ui:
config_file: ./cmd/jaeger/config-ui.json

jaeger_storage:
backends:
Expand Down
3 changes: 2 additions & 1 deletion cmd/jaeger/config-kafka-ingester.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ extensions:
endpoint: 0.0.0.0:14133

jaeger_query:
trace_storage: some_storage
storage:
traces: some_storage

jaeger_storage:
backends:
Expand Down
8 changes: 5 additions & 3 deletions cmd/jaeger/config-opensearch.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@ extensions:
http:

jaeger_query:
trace_storage: some_storage
trace_storage_archive: another_storage
ui_config: ./cmd/jaeger/config-ui.json
storage:
traces: some_storage
traces_archive: another_storage
ui:
config_file: ./cmd/jaeger/config-ui.json

jaeger_storage:
backends:
Expand Down
6 changes: 4 additions & 2 deletions cmd/jaeger/config-remote-storage.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ extensions:
http:

jaeger_query:
trace_storage: some-storage
ui_config: ./cmd/jaeger/config-ui.json
storage:
traces: some-storage
ui:
config_file: ./cmd/jaeger/config-ui.json

jaeger_storage:
backends:
Expand Down
3 changes: 2 additions & 1 deletion cmd/jaeger/config-tail-sampling-always-sample.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ extensions:
use_v2: true
http:
jaeger_query:
trace_storage: some_storage
storage:
traces: some_storage
jaeger_storage:
backends:
some_storage:
Expand Down
3 changes: 2 additions & 1 deletion cmd/jaeger/config-tail-sampling-service-name-policy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ extensions:
use_v2: true
http:
jaeger_query:
trace_storage: some_storage
storage:
traces: some_storage
jaeger_storage:
backends:
some_storage:
Expand Down
8 changes: 5 additions & 3 deletions cmd/jaeger/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@ extensions:
# endpoint: 0.0.0.0:55679

jaeger_query:
trace_storage: some_store
trace_storage_archive: another_store
ui_config: ./cmd/jaeger/config-ui.json
storage:
traces: some_store
traces_archive: another_store
ui:
config_file: ./cmd/jaeger/config-ui.json

jaeger_storage:
backends:
Expand Down
3 changes: 2 additions & 1 deletion cmd/jaeger/internal/all-in-one.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ service:

extensions:
jaeger_query:
trace_storage: some_storage
storage:
traces: some_storage

jaeger_storage:
backends:
Expand Down
22 changes: 15 additions & 7 deletions cmd/jaeger/internal/extension/jaegerquery/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,28 @@ import (
"go.opentelemetry.io/collector/config/confighttp"

queryApp "github.com/jaegertracing/jaeger/cmd/query/app"
"github.com/jaegertracing/jaeger/pkg/tenancy"
)

var _ component.ConfigValidator = (*Config)(nil)

// Config represents the configuration for jaeger-query,
type Config struct {
queryApp.QueryOptionsBase `mapstructure:",squash"`
TraceStoragePrimary string `valid:"required" mapstructure:"trace_storage"`
TraceStorageArchive string `valid:"optional" mapstructure:"trace_storage_archive"`
MetricStorage string `valid:"optional" mapstructure:"metric_storage"`
HTTP confighttp.ServerConfig `mapstructure:",squash"`
GRPC configgrpc.ServerConfig `mapstructure:",squash"`
Tenancy tenancy.Options `mapstructure:"multi_tenancy"`
// Storage holds configuration related to the various data stores that are to be queried.
Storage Storage `mapstructure:"storage"`
// HTTP holds the HTTP configuration that the query service uses to serve requests.
HTTP confighttp.ServerConfig `mapstructure:"http"`
// GRPC holds the GRPC configuration that the query service uses to serve requests.
GRPC configgrpc.ServerConfig `mapstructure:"grpc"`
}

type Storage struct {
// TracesPrimary contains the name of the primary trace storage that is being queried.
TracesPrimary string `mapstructure:"traces" valid:"required"`
// TracesArchive contains the name of the archive trace storage that is being queried.
TracesArchive string `mapstructure:"traces_archive" valid:"optional"`
// Metrics contains the name of the metric storage that is being queried.
Metrics string `mapstructure:"metrics" valid:"optional"`
}

func (cfg *Config) Validate() error {
Expand Down
6 changes: 4 additions & 2 deletions cmd/jaeger/internal/extension/jaegerquery/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@ func Test_Validate(t *testing.T) {
{
name: "Empty config",
config: &Config{},
expectedErr: "TraceStoragePrimary: non zero value required",
expectedErr: "Storage.TracesPrimary: non zero value required",
},
{
name: "Non empty-config",
config: &Config{
TraceStoragePrimary: "some-storage",
Storage: Storage{
TracesPrimary: "some-storage",
},
},
expectedErr: "",
},
Expand Down
12 changes: 6 additions & 6 deletions cmd/jaeger/internal/extension/jaegerquery/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,9 @@ func (s *server) Start(_ context.Context, host component.Host) error {
mf := otelmetrics.NewFactory(s.telset.MeterProvider)
baseFactory := mf.Namespace(metrics.NSOptions{Name: "jaeger"})
queryMetricsFactory := baseFactory.Namespace(metrics.NSOptions{Name: "query"})
f, err := jaegerstorage.GetStorageFactory(s.config.TraceStoragePrimary, host)
f, err := jaegerstorage.GetStorageFactory(s.config.Storage.TracesPrimary, host)
if err != nil {
return fmt.Errorf("cannot find primary storage %s: %w", s.config.TraceStoragePrimary, err)
return fmt.Errorf("cannot find primary storage %s: %w", s.config.Storage.TracesPrimary, err)
}

spanReader, err := f.CreateSpanReader()
Expand Down Expand Up @@ -124,12 +124,12 @@ func (s *server) Start(_ context.Context, host component.Host) error {
}

func (s *server) addArchiveStorage(opts *querysvc.QueryServiceOptions, host component.Host) error {
if s.config.TraceStorageArchive == "" {
if s.config.Storage.TracesArchive == "" {
s.telset.Logger.Info("Archive storage not configured")
return nil
}

f, err := jaegerstorage.GetStorageFactory(s.config.TraceStorageArchive, host)
f, err := jaegerstorage.GetStorageFactory(s.config.Storage.TracesArchive, host)
if err != nil {
return fmt.Errorf("cannot find archive storage factory: %w", err)
}
Expand All @@ -141,12 +141,12 @@ func (s *server) addArchiveStorage(opts *querysvc.QueryServiceOptions, host comp
}

func (s *server) createMetricReader(host component.Host) (metricsstore.Reader, error) {
if s.config.MetricStorage == "" {
if s.config.Storage.Metrics == "" {
s.telset.Logger.Info("Metric storage not configured")
return disabled.NewMetricsReader()
}

mf, err := jaegerstorage.GetMetricsFactory(s.config.MetricStorage, host)
mf, err := jaegerstorage.GetMetricsFactory(s.config.Storage.Metrics, host)
if err != nil {
return nil, fmt.Errorf("cannot find metrics storage factory: %w", err)
}
Expand Down
50 changes: 35 additions & 15 deletions cmd/jaeger/internal/extension/jaegerquery/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,53 +134,67 @@ func TestServerStart(t *testing.T) {
{
name: "Non-empty config with fake storage host",
config: &Config{
TraceStorageArchive: "jaeger_storage",
TraceStoragePrimary: "jaeger_storage",
MetricStorage: "jaeger_metrics_storage",
Storage: Storage{
TracesArchive: "jaeger_storage",
TracesPrimary: "jaeger_storage",
Metrics: "jaeger_metrics_storage",
},
},
},
{
name: "factory error",
config: &Config{
TraceStoragePrimary: "need-factory-error",
Storage: Storage{
TracesPrimary: "need-factory-error",
},
},
expectedErr: "cannot find primary storage",
},
{
name: "span reader error",
config: &Config{
TraceStoragePrimary: "need-span-reader-error",
Storage: Storage{
TracesPrimary: "need-span-reader-error",
},
},
expectedErr: "cannot create span reader",
},
{
name: "dependency error",
config: &Config{
TraceStoragePrimary: "need-dependency-reader-error",
Storage: Storage{
TracesPrimary: "need-dependency-reader-error",
},
},
expectedErr: "cannot create dependencies reader",
},
{
name: "storage archive error",
config: &Config{
TraceStorageArchive: "need-factory-error",
TraceStoragePrimary: "jaeger_storage",
Storage: Storage{
TracesArchive: "need-factory-error",
TracesPrimary: "jaeger_storage",
},
},
expectedErr: "cannot find archive storage factory",
},
{
name: "metrics storage error",
config: &Config{
MetricStorage: "need-factory-error",
TraceStoragePrimary: "jaeger_storage",
Storage: Storage{
Metrics: "need-factory-error",
TracesPrimary: "jaeger_storage",
},
},
expectedErr: "cannot find metrics storage factory",
},
{
name: " metrics reader error",
config: &Config{
MetricStorage: "need-metrics-reader-error",
TraceStoragePrimary: "jaeger_storage",
Storage: Storage{
Metrics: "need-metrics-reader-error",
TracesPrimary: "jaeger_storage",
},
},
expectedErr: "cannot create metrics reader",
},
Expand Down Expand Up @@ -254,7 +268,9 @@ func TestServerAddArchiveStorage(t *testing.T) {
{
name: "Archive storage set",
config: &Config{
TraceStorageArchive: "random-value",
Storage: Storage{
TracesArchive: "random-value",
},
},
qSvcOpts: &querysvc.QueryServiceOptions{},
expectedOutput: "",
Expand All @@ -263,7 +279,9 @@ func TestServerAddArchiveStorage(t *testing.T) {
{
name: "Archive storage not supported",
config: &Config{
TraceStorageArchive: "badger",
Storage: Storage{
TracesArchive: "badger",
},
},
qSvcOpts: &querysvc.QueryServiceOptions{},
extension: fakeStorageExt{},
Expand Down Expand Up @@ -313,7 +331,9 @@ func TestServerAddMetricsStorage(t *testing.T) {
{
name: "Metrics storage set",
config: &Config{
MetricStorage: "random-value",
Storage: Storage{
Metrics: "random-value",
},
},
expectedOutput: "",
expectedErr: "cannot find metrics storage factory: cannot find extension",
Expand Down
6 changes: 4 additions & 2 deletions cmd/jaeger/internal/integration/e2e_integration.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ func (s *E2EStorageIntegration) e2eInitialize(t *testing.T, storage string) {
Path: "./cmd/jaeger/jaeger",
Args: []string{"jaeger", "--config", configFile},
// Change the working directory to the root of this project
// since the binary config file jaeger_query's ui_config points to
// since the binary config file jaeger_query's ui.config_file points to
// "./cmd/jaeger/config-ui.json"
Dir: "../../../..",
},
Expand Down Expand Up @@ -223,7 +223,9 @@ func createStorageCleanerConfig(t *testing.T, configFile string, storage string)
extensions := extensionsAny.(map[string]any)
queryAny, ok := extensions["jaeger_query"]
require.True(t, ok)
traceStorageAny, ok := queryAny.(map[string]any)["trace_storage"]
storageAny, ok := queryAny.(map[string]any)["storage"]
require.True(t, ok)
traceStorageAny, ok := storageAny.(map[string]any)["traces"]
require.True(t, ok)
traceStorage := traceStorageAny.(string)
extensions["storage_cleaner"] = map[string]string{"trace_storage": traceStorage}
Expand Down
Loading

0 comments on commit d7f01d1

Please sign in to comment.