Skip to content

Commit 48d374c

Browse files
Copilotyurishkuro
andauthored
Migrate remote-storage to YAML configuration with shared storageconfig package (#7704)
## Migrated remote-storage to YAML configuration with shared storageconfig package Addressed all feedback from @yurishkuro to eliminate duplication and maintain test coverage. ### Changes Made: 1. **Shared Storage Configuration with Comprehensive Tests** (`cmd/internal/storageconfig/`) - Created shared package with `Config`, `TraceBackend`, and `MetricBackend` types - **PrometheusConfiguration now in storageconfig** with `promcfg.Configuration` type - **MetricBackend.Unmarshal in storageconfig** initializes prometheus defaults - Implemented `CreateTraceStorageFactory` with optional auth support - Both jaeger and remote-storage now use shared factory creation logic - **Added comprehensive unit tests** (`config_test.go`, `factory_test.go`) - **Config validation and unmarshaling: 100% test coverage** - **Factory creation tested for all backend types** (Memory, Badger, Cassandra, ClickHouse, Elasticsearch, OpenSearch) - **Total package coverage: 98.3%** (config.go: 100%, factory.go: 96.2%) - ES/OS backends tested with mock HTTP servers - ClickHouse tested with clickhousetest server - GRPC tested via jaegerstorage extension (requires component.Host) 2. **Refactored jaegerstorage Extension** (`cmd/jaeger/internal/extension/jaegerstorage/`) - **Config now embeds storageconfig.Config** with mapstructure squash - Extension uses shared `storageconfig.CreateTraceStorageFactory` - Eliminated ~80 lines of duplicate code (config types + factory creation) - Maintained 99.1% test coverage (previously 99.4%) - No duplicate PrometheusConfiguration or MetricBackend definitions 3. **Updated All Test Files** - Migrated all test references to use embedded Config initialization - Fixed references in extension, exporter, processor, and remote sampling tests - All tests passing with high coverage 4. **Enhanced remote-storage Configuration** - Uses viper's built-in `--config-file` flag - Removed all CLI flag support and v1 factory dependencies - Added validation to enforce single backend constraint - Added default configuration support (memory storage on :17271) - Maintains backward compatibility with integration tests ### Testing Results: - ✅ **storageconfig package: 98.3% coverage** (config.go: 100%, factory.go: 96.2%) - ✅ jaegerstorage extension: 99.1% coverage - ✅ All exporters, processors: 100% coverage - ✅ remote-storage: 94% coverage - ✅ All test suites passing - ✅ Lint clean - ✅ remote-storage starts successfully without config file ### Benefits: - ✅ No duplication - all config types in shared storageconfig - ✅ **Comprehensive test coverage for shared code** (98.3%) - ✅ Single source of truth for storage configuration - ✅ High test coverage maintained across all packages - ✅ Clean architecture with embedded config - ✅ Backward compatible with existing deployments <!-- START COPILOT CODING AGENT SUFFIX --> <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > Can you solve this issue #7703 ? </details> <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Signed-off-by: Yuri Shkuro <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]> Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: yurishkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>
1 parent 0b71b4a commit 48d374c

File tree

30 files changed

+1532
-482
lines changed

30 files changed

+1532
-482
lines changed

Makefile

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ ALL_SRC = $(shell find . -name '*.go' \
4040
# All .sh or .py or Makefile or .mk files that should be auto-formatted and linted.
4141
SCRIPTS_SRC = $(shell find . \( -name '*.sh' -o -name '*.py' -o -name '*.mk' -o -name 'Makefile*' -o -name 'Dockerfile*' \) \
4242
-not -path './.git/*' \
43+
-not -path './vendor/*' \
4344
-not -path './idl/*' \
4445
-not -path './jaeger-ui/*' \
4546
-type f | \

cmd/internal/flags/flags.go

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,9 @@ import (
1616
)
1717

1818
const (
19-
spanStorageType = "span-storage.type" // deprecated
20-
logLevel = "log-level"
21-
logEncoding = "log-encoding" // json or console
22-
configFile = "config-file"
19+
logLevel = "log-level"
20+
logEncoding = "log-encoding" // json or console
21+
configFile = "config-file"
2322
)
2423

2524
// AddConfigFileFlag adds flags for ExternalConfFlags
@@ -92,12 +91,6 @@ type logging struct {
9291
Encoding string
9392
}
9493

95-
// AddFlags adds flags for SharedFlags
96-
func AddFlags(flagSet *flag.FlagSet) {
97-
flagSet.String(spanStorageType, "", "(deprecated) please use SPAN_STORAGE_TYPE environment variable. Run this binary with the 'env' command for help.")
98-
AddLoggingFlags(flagSet)
99-
}
100-
10194
// AddLoggingFlag adds logging flag for SharedFlags
10295
func AddLoggingFlags(flagSet *flag.FlagSet) {
10396
flagSet.String(logLevel, "info", "Minimal allowed log Level. For more levels see https://github.com/uber-go/zap")

cmd/internal/flags/service.go

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,6 @@ type Service struct {
2828
// AdminPort is the HTTP port number for admin server.
2929
AdminPort int
3030

31-
// NoStorage indicates that storage-type CLI flag is not applicable
32-
NoStorage bool
33-
3431
// Admin is the admin server that hosts the health check and metrics endpoints.
3532
Admin *AdminServer
3633

@@ -80,11 +77,7 @@ func NewService(adminPort int) *Service {
8077
// AddFlags registers CLI flags.
8178
func (s *Service) AddFlags(flagSet *flag.FlagSet) {
8279
AddConfigFileFlag(flagSet)
83-
if s.NoStorage {
84-
AddLoggingFlags(flagSet)
85-
} else {
86-
AddFlags(flagSet)
87-
}
80+
AddLoggingFlags(flagSet)
8881
metricsbuilder.AddFlags(flagSet)
8982
s.Admin.AddFlags(flagSet)
9083
featuregate.GlobalRegistry().RegisterFlags(flagSet)

cmd/internal/flags/service_test.go

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,6 @@ import (
2121
func TestAddFlags(*testing.T) {
2222
s := NewService(0)
2323
s.AddFlags(new(flag.FlagSet))
24-
25-
s.NoStorage = true
26-
s.AddFlags(new(flag.FlagSet))
2724
}
2825

2926
func TestStartErrors(t *testing.T) {
Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
// Copyright (c) 2025 The Jaeger Authors.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package storageconfig
5+
6+
import (
7+
"errors"
8+
"fmt"
9+
"reflect"
10+
"time"
11+
12+
"go.opentelemetry.io/collector/confmap"
13+
14+
"github.com/jaegertracing/jaeger/internal/config/promcfg"
15+
cascfg "github.com/jaegertracing/jaeger/internal/storage/cassandra/config"
16+
escfg "github.com/jaegertracing/jaeger/internal/storage/elasticsearch/config"
17+
"github.com/jaegertracing/jaeger/internal/storage/metricstore/prometheus"
18+
"github.com/jaegertracing/jaeger/internal/storage/v1/badger"
19+
"github.com/jaegertracing/jaeger/internal/storage/v1/cassandra"
20+
es "github.com/jaegertracing/jaeger/internal/storage/v1/elasticsearch"
21+
"github.com/jaegertracing/jaeger/internal/storage/v1/memory"
22+
"github.com/jaegertracing/jaeger/internal/storage/v2/clickhouse"
23+
"github.com/jaegertracing/jaeger/internal/storage/v2/grpc"
24+
)
25+
26+
var (
27+
_ confmap.Unmarshaler = (*TraceBackend)(nil)
28+
_ confmap.Unmarshaler = (*MetricBackend)(nil)
29+
)
30+
31+
// Config contains configuration(s) for Jaeger trace storage.
32+
type Config struct {
33+
TraceBackends map[string]TraceBackend `mapstructure:"backends"`
34+
MetricBackends map[string]MetricBackend `mapstructure:"metric_backends"`
35+
}
36+
37+
// TraceBackend contains configuration for a single trace storage backend.
38+
type TraceBackend struct {
39+
Memory *memory.Configuration `mapstructure:"memory"`
40+
Badger *badger.Config `mapstructure:"badger"`
41+
GRPC *grpc.Config `mapstructure:"grpc"`
42+
Cassandra *cassandra.Options `mapstructure:"cassandra"`
43+
Elasticsearch *escfg.Configuration `mapstructure:"elasticsearch"`
44+
Opensearch *escfg.Configuration `mapstructure:"opensearch"`
45+
ClickHouse *clickhouse.Configuration `mapstructure:"clickhouse"`
46+
}
47+
48+
// MetricBackend contains configuration for a single metric storage backend.
49+
type MetricBackend struct {
50+
Prometheus *PrometheusConfiguration `mapstructure:"prometheus"`
51+
Elasticsearch *escfg.Configuration `mapstructure:"elasticsearch"`
52+
Opensearch *escfg.Configuration `mapstructure:"opensearch"`
53+
}
54+
55+
type PrometheusConfiguration struct {
56+
Configuration promcfg.Configuration `mapstructure:",squash"`
57+
Authentication escfg.Authentication `mapstructure:"auth"`
58+
}
59+
60+
// Unmarshal implements confmap.Unmarshaler. This allows us to provide
61+
// defaults for different configs.
62+
func (cfg *TraceBackend) Unmarshal(conf *confmap.Conf) error {
63+
// apply defaults
64+
if conf.IsSet("memory") {
65+
cfg.Memory = &memory.Configuration{
66+
MaxTraces: 1_000_000,
67+
}
68+
}
69+
if conf.IsSet("badger") {
70+
v := badger.DefaultConfig()
71+
cfg.Badger = v
72+
}
73+
if conf.IsSet("grpc") {
74+
v := grpc.DefaultConfig()
75+
cfg.GRPC = &v
76+
}
77+
if conf.IsSet("cassandra") {
78+
cfg.Cassandra = &cassandra.Options{
79+
NamespaceConfig: cassandra.NamespaceConfig{
80+
Configuration: cascfg.DefaultConfiguration(),
81+
Enabled: true,
82+
},
83+
SpanStoreWriteCacheTTL: 12 * time.Hour,
84+
Index: cassandra.IndexConfig{
85+
Tags: true,
86+
ProcessTags: true,
87+
Logs: true,
88+
},
89+
}
90+
}
91+
if conf.IsSet("elasticsearch") {
92+
v := es.DefaultConfig()
93+
cfg.Elasticsearch = &v
94+
}
95+
if conf.IsSet("opensearch") {
96+
v := es.DefaultConfig()
97+
cfg.Opensearch = &v
98+
}
99+
return conf.Unmarshal(cfg)
100+
}
101+
102+
// Unmarshal implements confmap.Unmarshaler for MetricBackend.
103+
func (cfg *MetricBackend) Unmarshal(conf *confmap.Conf) error {
104+
// apply defaults
105+
if conf.IsSet("prometheus") {
106+
v := prometheus.DefaultConfig()
107+
cfg.Prometheus = &PrometheusConfiguration{
108+
Configuration: v,
109+
}
110+
}
111+
if conf.IsSet("elasticsearch") {
112+
v := es.DefaultConfig()
113+
cfg.Elasticsearch = &v
114+
}
115+
if conf.IsSet("opensearch") {
116+
v := es.DefaultConfig()
117+
cfg.Opensearch = &v
118+
}
119+
return conf.Unmarshal(cfg)
120+
}
121+
122+
// Validate validates the storage configuration.
123+
func (c *Config) Validate() error {
124+
if len(c.TraceBackends) == 0 {
125+
return errors.New("at least one storage backend is required")
126+
}
127+
for name, b := range c.TraceBackends {
128+
empty := TraceBackend{}
129+
if reflect.DeepEqual(b, empty) {
130+
return fmt.Errorf("empty backend configuration for storage '%s'", name)
131+
}
132+
}
133+
return nil
134+
}

0 commit comments

Comments
 (0)