Skip to content

Commit b376dc3

Browse files
hharshhsainihharshhsaini
committed
refactor(query): extract UI StorageCapabilities resolution to factory caller
Signed-off-by: hharshhsaini <sainiharsh3311@gmail.com>
1 parent 65bdc39 commit b376dc3

6 files changed

Lines changed: 147 additions & 161 deletions

File tree

‎cmd/jaeger/internal/extension/jaegerquery/internal/server.go‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ import (
3131
"github.com/jaegertracing/jaeger/internal/auth/bearertoken"
3232
"github.com/jaegertracing/jaeger/internal/proto/api_v3"
3333
"github.com/jaegertracing/jaeger/internal/recoveryhandler"
34-
"github.com/jaegertracing/jaeger/internal/storage/metricstore/disabled"
3534
"github.com/jaegertracing/jaeger/internal/storage/v1/api/metricstore"
3635
"github.com/jaegertracing/jaeger/internal/telemetry"
3736
"github.com/jaegertracing/jaeger/internal/tenancy"
@@ -54,6 +53,7 @@ func NewServer(
5453
querySvc *querysvc.QueryService,
5554
metricsQuerySvc metricstore.Reader,
5655
options *QueryOptions,
56+
caps querysvc.StorageCapabilities,
5757
tm *tenancy.Manager,
5858
telset telemetry.Settings,
5959
) (*Server, error) {
@@ -76,7 +76,7 @@ func NewServer(
7676
return nil, err
7777
}
7878
registerGRPCHandlers(grpcServer, querySvc, telset)
79-
httpServer, err := createHTTPServer(ctx, querySvc, metricsQuerySvc, options, tm, telset)
79+
httpServer, err := createHTTPServer(ctx, querySvc, metricsQuerySvc, options, caps, tm, telset)
8080
if err != nil {
8181
return nil, err
8282
}
@@ -158,6 +158,7 @@ func initRouter(
158158
querySvc *querysvc.QueryService,
159159
metricsQuerySvc metricstore.Reader,
160160
queryOpts *QueryOptions,
161+
caps querysvc.StorageCapabilities,
161162
tenancyMgr *tenancy.Manager,
162163
telset telemetry.Settings,
163164
) (http.Handler, io.Closer) {
@@ -192,9 +193,6 @@ func initRouter(
192193
http.Error(w, "404 page not found", http.StatusNotFound)
193194
})
194195

195-
caps := querySvc.GetCapabilities()
196-
_, disabledMetrics := metricsQuerySvc.(*disabled.MetricsReader)
197-
caps.MetricsStorage = metricsQuerySvc != nil && !disabledMetrics
198196
staticHandlerCloser := RegisterStaticHandler(r, telset.Logger, queryOpts, caps)
199197

200198
var handler http.Handler = r
@@ -213,10 +211,11 @@ func createHTTPServer(
213211
querySvc *querysvc.QueryService,
214212
metricsQuerySvc metricstore.Reader,
215213
queryOpts *QueryOptions,
214+
caps querysvc.StorageCapabilities,
216215
tm *tenancy.Manager,
217216
telset telemetry.Settings,
218217
) (*httpServer, error) {
219-
handler, staticHandlerCloser := initRouter(querySvc, metricsQuerySvc, queryOpts, tm, telset)
218+
handler, staticHandlerCloser := initRouter(querySvc, metricsQuerySvc, queryOpts, caps, tm, telset)
220219
handler = recoveryhandler.NewRecoveryHandler(telset.Logger, true)(handler)
221220
var extensions map[component.ID]component.Component
222221
if telset.Host != nil {

‎cmd/jaeger/internal/extension/jaegerquery/internal/server_test.go‎

Lines changed: 72 additions & 116 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,7 @@ import (
3737
"github.com/jaegertracing/jaeger-idl/proto-gen/api_v2"
3838
"github.com/jaegertracing/jaeger/cmd/jaeger/internal/extension/jaegerquery/querysvc"
3939
"github.com/jaegertracing/jaeger/internal/grpctest"
40-
"github.com/jaegertracing/jaeger/internal/proto-gen/api_v2/metrics"
4140
"github.com/jaegertracing/jaeger/internal/storage/metricstore/disabled"
42-
"github.com/jaegertracing/jaeger/internal/storage/v1/api/metricstore"
4341
depsmocks "github.com/jaegertracing/jaeger/internal/storage/v2/api/depstore/mocks"
4442
"github.com/jaegertracing/jaeger/internal/storage/v2/api/tracestore"
4543
tracestoremocks "github.com/jaegertracing/jaeger/internal/storage/v2/api/tracestore/mocks"
@@ -92,6 +90,7 @@ func TestCreateTLSServerSinglePortError(t *testing.T) {
9290
},
9391
GRPC: configgrpc.ServerConfig{NetAddr: confignet.AddrConfig{Endpoint: ":8080", Transport: confignet.TransportTypeTCP}, TLS: configoptional.Some(tlsCfg)},
9492
},
93+
querysvc.StorageCapabilities{},
9594
tenancy.NewManager(&tenancy.Options{}), telset)
9695
require.Error(t, err)
9796
}
@@ -115,6 +114,7 @@ func TestCreateTLSGrpcServerError(t *testing.T) {
115114
},
116115
GRPC: configgrpc.ServerConfig{NetAddr: confignet.AddrConfig{Endpoint: ":8081", Transport: confignet.TransportTypeTCP}, TLS: configoptional.Some(tlsCfg)},
117116
},
117+
querysvc.StorageCapabilities{},
118118
tenancy.NewManager(&tenancy.Options{}), telset)
119119
require.Error(t, err)
120120
}
@@ -138,7 +138,9 @@ func TestStartTLSHttpServerError(t *testing.T) {
138138
TLS: configoptional.Some(tlsCfg),
139139
},
140140
GRPC: configgrpc.ServerConfig{NetAddr: confignet.AddrConfig{Endpoint: ":8081", Transport: confignet.TransportTypeTCP}},
141-
}, tenancy.NewManager(&tenancy.Options{}), telset)
141+
},
142+
querysvc.StorageCapabilities{},
143+
tenancy.NewManager(&tenancy.Options{}), telset)
142144
require.NoError(t, err)
143145
require.Error(t, s.Start(context.Background()))
144146
t.Cleanup(func() {
@@ -437,9 +439,9 @@ func TestServerHTTPTLS(t *testing.T) {
437439
logger := zaptest.NewLogger(t)
438440
telset := initTelSet(logger, nooptrace.NewTracerProvider())
439441
querySvc := makeQuerySvc()
440-
server, err := NewServer(context.Background(), querySvc.qs,
441-
nil, serverOptions, tenancy.NewManager(&tenancy.Options{}),
442-
telset)
442+
443+
server, err := NewServer(context.Background(), querySvc.qs, nil,
444+
serverOptions, querysvc.StorageCapabilities{}, tenancy.NewManager(&tenancy.Options{}), telset)
443445
require.NoError(t, err)
444446
require.NoError(t, server.Start(context.Background()))
445447
t.Cleanup(func() {
@@ -552,7 +554,7 @@ func TestServerGRPCTLS(t *testing.T) {
552554
querySvc := makeQuerySvc()
553555
telset := initTelSet(logger, nooptrace.NewTracerProvider())
554556
server, err := NewServer(context.Background(), querySvc.qs,
555-
nil, serverOptions, tenancy.NewManager(&tenancy.Options{}),
557+
nil, serverOptions, querysvc.StorageCapabilities{}, tenancy.NewManager(&tenancy.Options{}),
556558
telset)
557559
require.NoError(t, err)
558560
require.NoError(t, server.Start(context.Background()))
@@ -609,6 +611,7 @@ func TestServerBadHostPort(t *testing.T) {
609611
},
610612
},
611613
},
614+
querysvc.StorageCapabilities{},
612615
tenancy.NewManager(&tenancy.Options{}),
613616
telset)
614617
require.Error(t, err)
@@ -629,124 +632,79 @@ func TestServerBadHostPort(t *testing.T) {
629632
},
630633
},
631634
},
635+
querysvc.StorageCapabilities{},
632636
tenancy.NewManager(&tenancy.Options{}),
633637
telset)
634638

635639
require.Error(t, err)
636640
}
637641

638-
func TestServerMetricsStorageDisabled(t *testing.T) {
639-
serverOptions := &QueryOptions{
640-
HTTP: confighttp.ServerConfig{
641-
NetAddr: confignet.AddrConfig{
642-
Endpoint: ":0",
643-
Transport: confignet.TransportTypeTCP,
644-
},
645-
},
646-
GRPC: configgrpc.ServerConfig{
647-
NetAddr: confignet.AddrConfig{
648-
Endpoint: ":0",
649-
Transport: confignet.TransportTypeTCP,
650-
},
642+
func TestServerMetricsStorage(t *testing.T) {
643+
testCases := []struct {
644+
name string
645+
metricsStorageEnabled bool
646+
}{
647+
{
648+
name: "metrics enabled",
649+
metricsStorageEnabled: true,
651650
},
652-
UIConfig: UIConfig{
653-
AssetsPath: "fixture",
654-
ConfigFile: "fixture/ui-config.json",
651+
{
652+
name: "metrics disabled",
653+
metricsStorageEnabled: false,
655654
},
656655
}
657656

658-
logger := zaptest.NewLogger(t)
659-
telset := initTelSet(logger, nooptrace.NewTracerProvider())
660-
querySvc := makeQuerySvc()
661-
662-
disabledMetricsReader, err := disabled.NewMetricsReader()
663-
require.NoError(t, err)
664-
665-
server, err := NewServer(context.Background(), querySvc.qs, disabledMetricsReader, serverOptions, tenancy.NewManager(&tenancy.Options{}), telset)
666-
require.NoError(t, err)
667-
require.NoError(t, server.Start(context.Background()))
668-
defer server.Close()
669-
670-
client := &http.Client{}
671-
req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("http://%s/", server.HTTPAddr()), http.NoBody)
672-
require.NoError(t, err)
673-
resp, err := client.Do(req)
674-
require.NoError(t, err)
675-
defer resp.Body.Close()
676-
677-
require.Equal(t, http.StatusOK, resp.StatusCode)
678-
679-
htmlBytes, err := io.ReadAll(resp.Body)
680-
require.NoError(t, err)
657+
for _, tc := range testCases {
658+
t.Run(tc.name, func(t *testing.T) {
659+
serverOptions := &QueryOptions{
660+
HTTP: confighttp.ServerConfig{
661+
NetAddr: confignet.AddrConfig{
662+
Endpoint: ":0",
663+
Transport: confignet.TransportTypeTCP,
664+
},
665+
},
666+
GRPC: configgrpc.ServerConfig{
667+
NetAddr: confignet.AddrConfig{
668+
Endpoint: ":0",
669+
Transport: confignet.TransportTypeTCP,
670+
},
671+
},
672+
UIConfig: UIConfig{
673+
AssetsPath: "fixture",
674+
ConfigFile: "fixture/ui-config.json",
675+
},
676+
}
681677

682-
htmlText := string(htmlBytes)
683-
assert.Contains(t, htmlText, `"metricsStorage":false`, "Expected metricsStorage to be false for a disabled metrics reader.")
684-
}
678+
logger := zaptest.NewLogger(t)
679+
telset := initTelSet(logger, nooptrace.NewTracerProvider())
680+
querySvc := makeQuerySvc()
685681

686-
type fakeMetricsReader struct{}
682+
disabledReader, err := disabled.NewMetricsReader()
683+
require.NoError(t, err)
687684

688-
func (*fakeMetricsReader) GetCallRates(_ context.Context, _ *metricstore.CallRateQueryParameters) (*metrics.MetricFamily, error) {
689-
return nil, nil
690-
}
685+
server, err := NewServer(context.Background(), querySvc.qs, disabledReader, serverOptions, querysvc.StorageCapabilities{MetricsStorage: tc.metricsStorageEnabled}, tenancy.NewManager(&tenancy.Options{}), telset)
686+
require.NoError(t, err)
687+
require.NoError(t, server.Start(context.Background()))
688+
t.Cleanup(func() {
689+
require.NoError(t, server.Close())
690+
})
691691

692-
func (*fakeMetricsReader) GetErrorRates(_ context.Context, _ *metricstore.ErrorRateQueryParameters) (*metrics.MetricFamily, error) {
693-
return nil, nil
694-
}
692+
client := &http.Client{}
693+
req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("http://%s/", server.HTTPAddr()), http.NoBody)
694+
require.NoError(t, err)
695+
resp, err := client.Do(req)
696+
require.NoError(t, err)
697+
defer resp.Body.Close()
695698

696-
func (*fakeMetricsReader) GetLatencies(_ context.Context, _ *metricstore.LatenciesQueryParameters) (*metrics.MetricFamily, error) {
697-
return nil, nil
698-
}
699+
require.Equal(t, http.StatusOK, resp.StatusCode)
699700

700-
func (*fakeMetricsReader) GetMinStepDuration(_ context.Context, _ *metricstore.MinStepDurationQueryParameters) (time.Duration, error) {
701-
return 0, nil
702-
}
701+
htmlBytes, err := io.ReadAll(resp.Body)
702+
require.NoError(t, err)
703703

704-
func TestServerMetricsStorageEnabled(t *testing.T) {
705-
serverOptions := &QueryOptions{
706-
HTTP: confighttp.ServerConfig{
707-
NetAddr: confignet.AddrConfig{
708-
Endpoint: ":0",
709-
Transport: confignet.TransportTypeTCP,
710-
},
711-
},
712-
GRPC: configgrpc.ServerConfig{
713-
NetAddr: confignet.AddrConfig{
714-
Endpoint: ":0",
715-
Transport: confignet.TransportTypeTCP,
716-
},
717-
},
718-
UIConfig: UIConfig{
719-
AssetsPath: "fixture",
720-
ConfigFile: "fixture/ui-config.json",
721-
},
704+
expectedString := fmt.Sprintf(`"metricsStorage":%v`, tc.metricsStorageEnabled)
705+
assert.Contains(t, string(htmlBytes), expectedString, "Expected metricsStorage capability to reflect explicitly passed value.")
706+
})
722707
}
723-
724-
logger := zaptest.NewLogger(t)
725-
telset := initTelSet(logger, nooptrace.NewTracerProvider())
726-
querySvc := makeQuerySvc()
727-
728-
// Use our dummy, non-disabled MetricsReader
729-
fakeReader := &fakeMetricsReader{}
730-
731-
server, err := NewServer(context.Background(), querySvc.qs, fakeReader, serverOptions, tenancy.NewManager(&tenancy.Options{}), telset)
732-
require.NoError(t, err)
733-
require.NoError(t, server.Start(context.Background()))
734-
defer server.Close()
735-
736-
client := &http.Client{}
737-
req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("http://%s/", server.HTTPAddr()), http.NoBody)
738-
require.NoError(t, err)
739-
resp, err := client.Do(req)
740-
require.NoError(t, err)
741-
defer resp.Body.Close()
742-
743-
require.Equal(t, http.StatusOK, resp.StatusCode)
744-
745-
htmlBytes, err := io.ReadAll(resp.Body)
746-
require.NoError(t, err)
747-
748-
htmlText := string(htmlBytes)
749-
assert.Contains(t, htmlText, `"metricsStorage":true`, "Expected metricsStorage to be true for an active metrics reader.")
750708
}
751709

752710
func TestServerInUseHostPort(t *testing.T) {
@@ -784,6 +742,7 @@ func TestServerInUseHostPort(t *testing.T) {
784742
},
785743
},
786744
},
745+
querysvc.StorageCapabilities{},
787746
tenancy.NewManager(&tenancy.Options{}),
788747
telset,
789748
)
@@ -811,13 +770,9 @@ func TestServerGracefulExit(t *testing.T) {
811770
Transport: confignet.TransportTypeTCP,
812771
},
813772
},
814-
GRPC: configgrpc.ServerConfig{
815-
NetAddr: confignet.AddrConfig{
816-
Endpoint: ":0",
817-
Transport: confignet.TransportTypeTCP,
818-
},
819-
},
773+
GRPC: configgrpc.ServerConfig{NetAddr: confignet.AddrConfig{Endpoint: ":0", Transport: confignet.TransportTypeTCP}},
820774
},
775+
querysvc.StorageCapabilities{},
821776
tenancy.NewManager(&tenancy.Options{}), telset)
822777
require.NoError(t, err)
823778
require.NoError(t, server.Start(context.Background()))
@@ -863,6 +818,7 @@ func TestServerHandlesPortZero(t *testing.T) {
863818
},
864819
},
865820
},
821+
querysvc.StorageCapabilities{},
866822
tenancy.NewManager(&tenancy.Options{}),
867823
telset)
868824
require.NoError(t, err)
@@ -925,7 +881,7 @@ func TestServerHTTPTenancy(t *testing.T) {
925881
})).Once()
926882
telset := initTelSet(zaptest.NewLogger(t), nooptrace.NewTracerProvider())
927883
server, err := NewServer(context.Background(), querySvc.qs,
928-
nil, serverOptions, tenancyMgr, telset)
884+
nil, serverOptions, querysvc.StorageCapabilities{}, tenancyMgr, telset)
929885
require.NoError(t, err)
930886
require.NoError(t, server.Start(context.Background()))
931887
t.Cleanup(func() {
@@ -1025,7 +981,7 @@ func TestServerHTTP_TracesRequest(t *testing.T) {
1025981
telset := initTelSet(zaptest.NewLogger(t), tracerProvider)
1026982

1027983
server, err := NewServer(context.Background(), querySvc.qs,
1028-
nil, serverOptions, tenancyMgr, telset)
984+
nil, serverOptions, querysvc.StorageCapabilities{}, tenancyMgr, telset)
1029985
require.NoError(t, err)
1030986
require.NoError(t, server.Start(context.Background()))
1031987
t.Cleanup(func() {
@@ -1073,7 +1029,7 @@ func TestServerAPINotFound(t *testing.T) {
10731029
tenancyMgr := tenancy.NewManager(&serverOptions.Tenancy)
10741030
telset := initTelSet(zaptest.NewLogger(t), nooptrace.NewTracerProvider())
10751031

1076-
server, err := NewServer(context.Background(), querySvc.qs, nil, serverOptions, tenancyMgr, telset)
1032+
server, err := NewServer(context.Background(), querySvc.qs, nil, serverOptions, querysvc.StorageCapabilities{}, tenancyMgr, telset)
10771033
require.NoError(t, err)
10781034
require.NoError(t, server.Start(context.Background()))
10791035
t.Cleanup(func() {

‎cmd/jaeger/internal/extension/jaegerquery/querysvc/service.go‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -199,16 +199,6 @@ func (qs QueryService) GetDependencies(ctx context.Context, endTs time.Time, loo
199199
})
200200
}
201201

202-
func (qs QueryService) GetCapabilities() StorageCapabilities {
203-
return StorageCapabilities{
204-
ArchiveStorage: qs.options.hasArchiveStorage(),
205-
}
206-
}
207-
208-
func (opts *QueryServiceOptions) hasArchiveStorage() bool {
209-
return opts.ArchiveTraceReader != nil && opts.ArchiveTraceWriter != nil
210-
}
211-
212202
func (qs QueryService) receiveTraces(
213203
seq iter.Seq2[[]ptrace.Traces, error],
214204
yield func([]ptrace.Traces, error) bool,

0 commit comments

Comments
 (0)