Skip to content

Commit

Permalink
Address Feedback From PR Review
Browse files Browse the repository at this point in the history
Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>
  • Loading branch information
mahadzaryab1 committed Sep 22, 2024
1 parent c3e130d commit 5f70fd7
Show file tree
Hide file tree
Showing 8 changed files with 37 additions and 37 deletions.
8 changes: 4 additions & 4 deletions cmd/jaeger/internal/extension/jaegerquery/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@ type Connection struct {
}

type Storage struct {
// TracePrimary contains the name of the primary trace storage that is being queried.
TracePrimary string `mapstructure:"trace" valid:"required"`
// TracesPrimary contains the name of the primary trace storage that is being queried.
TracesPrimary string `mapstructure:"traces" valid:"required"`
// TraceArchive contains the name of the archive trace storage that is being queried.
TraceArchive string `mapstructure:"trace_archive" valid:"optional"`
// Metric contains the name of the metric storage that is being queried.
Metric string `mapstructure:"metric" 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
2 changes: 1 addition & 1 deletion cmd/jaeger/internal/extension/jaegerquery/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func Test_Validate(t *testing.T) {
name: "Non empty-config",
config: &Config{
Storage: Storage{
TracePrimary: "some-storage",
TracesPrimary: "some-storage",
},
},
expectedErr: "",
Expand Down
8 changes: 4 additions & 4 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.Storage.TracePrimary, host)
f, err := jaegerstorage.GetStorageFactory(s.config.Storage.TracesPrimary, host)
if err != nil {
return fmt.Errorf("cannot find primary storage %s: %w", s.config.Storage.TracePrimary, err)
return fmt.Errorf("cannot find primary storage %s: %w", s.config.Storage.TracesPrimary, err)
}

spanReader, err := f.CreateSpanReader()
Expand Down Expand Up @@ -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.Storage.Metric == "" {
if s.config.Storage.Metrics == "" {
s.telset.Logger.Info("Metric storage not configured")
return disabled.NewMetricsReader()
}

mf, err := jaegerstorage.GetMetricsFactory(s.config.Storage.Metric, 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
26 changes: 13 additions & 13 deletions cmd/jaeger/internal/extension/jaegerquery/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,17 +135,17 @@ func TestServerStart(t *testing.T) {
name: "Non-empty config with fake storage host",
config: &Config{
Storage: Storage{
TraceArchive: "jaeger_storage",
TracePrimary: "jaeger_storage",
Metric: "jaeger_metrics_storage",
TraceArchive: "jaeger_storage",
TracesPrimary: "jaeger_storage",
Metrics: "jaeger_metrics_storage",
},
},
},
{
name: "factory error",
config: &Config{
Storage: Storage{
TracePrimary: "need-factory-error",
TracesPrimary: "need-factory-error",
},
},
expectedErr: "cannot find primary storage",
Expand All @@ -154,7 +154,7 @@ func TestServerStart(t *testing.T) {
name: "span reader error",
config: &Config{
Storage: Storage{
TracePrimary: "need-span-reader-error",
TracesPrimary: "need-span-reader-error",
},
},
expectedErr: "cannot create span reader",
Expand All @@ -163,7 +163,7 @@ func TestServerStart(t *testing.T) {
name: "dependency error",
config: &Config{
Storage: Storage{
TracePrimary: "need-dependency-reader-error",
TracesPrimary: "need-dependency-reader-error",
},
},
expectedErr: "cannot create dependencies reader",
Expand All @@ -172,8 +172,8 @@ func TestServerStart(t *testing.T) {
name: "storage archive error",
config: &Config{
Storage: Storage{
TraceArchive: "need-factory-error",
TracePrimary: "jaeger_storage",
TraceArchive: "need-factory-error",
TracesPrimary: "jaeger_storage",
},
},
expectedErr: "cannot find archive storage factory",
Expand All @@ -182,8 +182,8 @@ func TestServerStart(t *testing.T) {
name: "metrics storage error",
config: &Config{
Storage: Storage{
Metric: "need-factory-error",
TracePrimary: "jaeger_storage",
Metrics: "need-factory-error",
TracesPrimary: "jaeger_storage",
},
},
expectedErr: "cannot find metrics storage factory",
Expand All @@ -192,8 +192,8 @@ func TestServerStart(t *testing.T) {
name: " metrics reader error",
config: &Config{
Storage: Storage{
Metric: "need-metrics-reader-error",
TracePrimary: "jaeger_storage",
Metrics: "need-metrics-reader-error",
TracesPrimary: "jaeger_storage",
},
},
expectedErr: "cannot create metrics reader",
Expand Down Expand Up @@ -332,7 +332,7 @@ func TestServerAddMetricsStorage(t *testing.T) {
name: "Metrics storage set",
config: &Config{
Storage: Storage{
Metric: "random-value",
Metrics: "random-value",
},
},
expectedOutput: "",
Expand Down
14 changes: 7 additions & 7 deletions cmd/query/app/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ var tlsHTTPFlagsConfig = tlscfg.ServerFlagsConfig{
Prefix: "query.http",
}

type UI struct {
type UIConfig struct {
// ConfigFile is the path to a configuration file for the UI.
ConfigFile string `mapstructure:"config_file" valid:"optional"`
// AssetsPath is the path for the static assets for the UI (https://github.com/uber/jaeger-ui).
Expand All @@ -61,14 +61,14 @@ type UI struct {
type QueryOptionsBase struct {
// BasePath is the base path for all HTTP routes.
BasePath string `mapstructure:"base_path"`
// UI contains configuration related to the Jaeger UI.
UI UI `mapstructure:"ui" valid:"optional"`
// UIConfig contains configuration related to the Jaeger UIConfig.
UIConfig UIConfig `mapstructure:"ui"`
// BearerTokenPropagation activate/deactivate bearer token propagation to storage.
BearerTokenPropagation bool `mapstructure:"bearer_token_propagation"`
// Tenancy holds the multi-tenancy configuration.
Tenancy tenancy.Options `mapstructure:"multi_tenancy"`
// MaxClockSkewAdjust is the maximum duration by which jaeger-query will adjust a span.
MaxClockSkewAdjust time.Duration `mapstructure:"max_clock_skew_adjust"`
MaxClockSkewAdjust time.Duration `mapstructure:"max_clock_skew_adjust" valid:"optional"`
// EnableTracing determines whether traces will be emitted by jaeger-query.
EnableTracing bool `mapstructure:"enable_tracing"`
}
Expand Down Expand Up @@ -120,9 +120,9 @@ func (qOpts *QueryOptions) InitFromViper(v *viper.Viper, logger *zap.Logger) (*Q
}
qOpts.TLSHTTP = tlsHTTP
qOpts.BasePath = v.GetString(queryBasePath)
qOpts.UI.AssetsPath = v.GetString(queryStaticFiles)
qOpts.UI.LogAccess = v.GetBool(queryLogStaticAssetsAccess)
qOpts.UI.ConfigFile = v.GetString(queryUIConfig)
qOpts.UIConfig.AssetsPath = v.GetString(queryStaticFiles)
qOpts.UIConfig.LogAccess = v.GetBool(queryLogStaticAssetsAccess)
qOpts.UIConfig.ConfigFile = v.GetString(queryUIConfig)
qOpts.BearerTokenPropagation = v.GetBool(queryTokenPropagation)

qOpts.MaxClockSkewAdjust = v.GetDuration(queryMaxClockSkewAdjust)
Expand Down
6 changes: 3 additions & 3 deletions cmd/query/app/flags_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ func TestQueryBuilderFlags(t *testing.T) {
})
qOpts, err := new(QueryOptions).InitFromViper(v, zap.NewNop())
require.NoError(t, err)
assert.Equal(t, "/dev/null", qOpts.UI.AssetsPath)
assert.True(t, qOpts.UI.LogAccess)
assert.Equal(t, "some.json", qOpts.UI.ConfigFile)
assert.Equal(t, "/dev/null", qOpts.UIConfig.AssetsPath)
assert.True(t, qOpts.UIConfig.LogAccess)
assert.Equal(t, "some.json", qOpts.UIConfig.ConfigFile)
assert.Equal(t, "/jaeger", qOpts.BasePath)
assert.Equal(t, "127.0.0.1:8080", qOpts.HTTPHostPort)
assert.Equal(t, "127.0.0.1:8081", qOpts.GRPCHostPort)
Expand Down
6 changes: 3 additions & 3 deletions cmd/query/app/static_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,12 @@ var (

// RegisterStaticHandler adds handler for static assets to the router.
func RegisterStaticHandler(r *mux.Router, logger *zap.Logger, qOpts *QueryOptions, qCapabilities querysvc.StorageCapabilities) io.Closer {
staticHandler, err := NewStaticAssetsHandler(qOpts.UI.AssetsPath, StaticAssetsHandlerOptions{
staticHandler, err := NewStaticAssetsHandler(qOpts.UIConfig.AssetsPath, StaticAssetsHandlerOptions{
BasePath: qOpts.BasePath,
UIConfigPath: qOpts.UI.ConfigFile,
UIConfigPath: qOpts.UIConfig.ConfigFile,
StorageCapabilities: qCapabilities,
Logger: logger,
LogAccess: qOpts.UI.LogAccess,
LogAccess: qOpts.UIConfig.LogAccess,
})
if err != nil {
logger.Panic("Could not create static assets handler", zap.Error(err))
Expand Down
4 changes: 2 additions & 2 deletions cmd/query/app/static_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func TestRegisterStaticHandlerPanic(t *testing.T) {
logger,
&QueryOptions{
QueryOptionsBase: QueryOptionsBase{
UI: UI{
UIConfig: UIConfig{
AssetsPath: "/foo/bar",
},
},
Expand Down Expand Up @@ -112,7 +112,7 @@ func TestRegisterStaticHandler(t *testing.T) {
}
closer := RegisterStaticHandler(r, logger, &QueryOptions{
QueryOptionsBase: QueryOptionsBase{
UI: UI{
UIConfig: UIConfig{
ConfigFile: testCase.UIConfigPath,
AssetsPath: "fixture",
LogAccess: testCase.logAccess,
Expand Down

0 comments on commit 5f70fd7

Please sign in to comment.