[WIP] [query] use otel's implementation for constructing http and grpc servers #6055

mahadzaryab1 · 2024-10-05T19:06:04Z

Which problem is this PR solving?

Resolves [query] Switch to use OTEL's http/grpc servers #6026

Description of the changes

How was this change tested?

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

codecov · 2024-10-05T19:13:58Z

Codecov Report

Attention: Patch coverage is 80.24691% with 16 lines in your changes missing coverage. Please review.

Project coverage is 96.81%. Comparing base (f411b3c) to head (0a7d837).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/query/app/server.go	79.74%	13 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6055      +/-   ##
==========================================
- Coverage   96.91%   96.81%   -0.11%     
==========================================
  Files         349      349              
  Lines       16587    16648      +61     
==========================================
+ Hits        16076    16117      +41     
- Misses        328      343      +15     
- Partials      183      188       +5

Flag	Coverage Δ
badger_v1	`7.99% <ø> (-0.01%)`	⬇️
badger_v2	`1.82% <ø> (-0.01%)`	⬇️
cassandra-4.x-v1	`15.79% <ø> (+0.01%)`	⬆️
cassandra-4.x-v2	`?`
cassandra-5.x-v1	`15.79% <ø> (+0.01%)`	⬆️
cassandra-5.x-v2	`1.74% <ø> (-0.01%)`	⬇️
elasticsearch-6.x-v1	`18.68% <ø> (-0.03%)`	⬇️
elasticsearch-7.x-v1	`18.74% <ø> (-0.04%)`	⬇️
elasticsearch-8.x-v1	`18.95% <ø> (-0.03%)`	⬇️
elasticsearch-8.x-v2	`1.80% <ø> (-0.02%)`	⬇️
grpc_v1	`9.37% <ø> (-0.02%)`	⬇️
grpc_v2	`?`
kafka-v1	`9.69% <ø> (-0.02%)`	⬇️
kafka-v2	`1.82% <ø> (-0.01%)`	⬇️
memory_v2	`1.82% <ø> (-0.01%)`	⬇️
opensearch-1.x-v1	`18.80% <ø> (-0.02%)`	⬇️
opensearch-2.x-v1	`18.79% <ø> (-0.04%)`	⬇️
opensearch-2.x-v2	`1.82% <ø> (+<0.01%)`	⬆️
tailsampling-processor	`0.46% <ø> (-0.01%)`	⬇️
unittests	`95.60% <80.24%> (-0.11%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mahadzaryab1 · 2024-10-05T19:17:30Z

@yurishkuro do you have any thoughts on how to handle the sharing of ports problem?

yurishkuro · 2024-10-05T21:57:43Z

We should follow our deprecation policy of two versions notice. It means for v2 we can go to new method and distinct port. For v1 we need to print a warning if the same port is used telling the user this will not be supported in the future (call it out in release notes). Then two releases later we switch default to not allow same port but still support it via a special flag (deprecated from start), and then two releases later remove that too.

This also means we should have another release tag "deprecated" and change release notes script to have a separate section for it.

mahadzaryab1 · 2024-10-05T22:01:09Z

We should follow our deprecation policy of two versions notice. It means for v2 we can go to new method and distinct port. For v1 we need to print a warning if the same port is used telling the user this will not be supported in the future (call it out in release notes). Then two releases later we switch default to not allow same port but still support it via a special flag (deprecated from start), and then two releases later remove that too.

This also means we should have another release tag "deprecated" and change release notes script to have a separate section for it.

Got it! Let me make some changes.

Signed-off-by: Mahad Zaryab <[email protected]>

mahadzaryab1 · 2024-10-05T23:39:05Z

@yurishkuro do you have any thoughts so far? it looks pretty messy but i'm not sure if there's a cleaner way to do this.

yurishkuro · 2024-10-06T20:17:26Z

cmd/query/app/server.go

 	metricsQuerySvc querysvc.MetricsQueryService,
 	options *QueryOptions,
 	tm *tenancy.Manager,
 	telset telemetery.Setting,
+	allowSamePort bool,


rather add it to QueryOptions

yurishkuro · 2024-10-06T20:18:00Z

cmd/query/app/server.go

-func NewServer(querySvc *querysvc.QueryService,
+func NewServer(
+	ctx context.Context,
+	host component.Host,


maybe we can wrap it into telemetery.Setting

yurishkuro · 2024-10-06T20:19:28Z

cmd/query/app/server.go

+			return nil, err
+		}
+	} else {
+		telset.Logger.Error("using the same port for gRPC and HTTP is deprecated; please use dedicated host ports intead")


Suggested change

telset.Logger.Error("using the same port for gRPC and HTTP is deprecated; please use dedicated host ports intead")

telset.Logger.Warning("using the same port for gRPC and HTTP is deprecated; please use dedicated ports instead")

yurishkuro · 2024-10-06T20:29:04Z

cmd/query/app/server.go

+	var grpcServer *grpc.Server
+	var httpServer *httpServer
+	if separatePorts {
+		grpcServer, err = createGRPCServer(ctx, host, querySvc, metricsQuerySvc, options, tm, telset)


I think this is unnecessary bifurcation at this point. I would instead spit this function into two, create and registerEndpoints(server). Pass legacy=separatePorts argument to create

func createGRPCServer(legacy bool, ... { if !legacy { return otel.Create... } // legacy code }

@yurishkuro how would we register the endpoints after creating the server?

Same way they are currently registered in the create() function. My point is that we're only changing how the server is created, not how endpoints are mounted into it.

@yurishkuro otel's ToServer takes in a handler, which has the routes registered to it. So how would we register the routes after creating the server using ToServer?

ok, so for grpc the order is (1) create server, (2) register handlers. For HTTP it's (1) create the root handler with all sub-handlers registered, and (2) create server. In both cases we can factor out the handlers creation, which are not different between v1/v2.

Signed-off-by: Mahad Zaryab <[email protected]>

mahadzaryab1 · 2024-10-09T03:06:23Z

@yurishkuro do you know why the all in one test is failing by any chance?

Signed-off-by: Mahad Zaryab <[email protected]>

mahadzaryab1 added 2 commits October 5, 2024 18:12

Pass Context To Server Functions

af14b4c

Signed-off-by: Mahad Zaryab <[email protected]>

Pass Host To NewServer Function

49f8261

Signed-off-by: Mahad Zaryab <[email protected]>

mahadzaryab1 force-pushed the otel-servers branch from 4f091a5 to 82044f5 Compare October 5, 2024 22:12

yurishkuro reviewed Oct 6, 2024

View reviewed changes

mahadzaryab1 added 3 commits October 8, 2024 21:16

Make Helper For Creating HTTP Router

b43f6ae

Signed-off-by: Mahad Zaryab <[email protected]>

Make Helper For Registering GRPC Server

8b4b4ca

Signed-off-by: Mahad Zaryab <[email protected]>

Add component.Host To telemetry.Setting

b0ee94f

Signed-off-by: Mahad Zaryab <[email protected]>

mahadzaryab1 force-pushed the otel-servers branch from 6f66c71 to b0ee94f Compare October 9, 2024 01:25

mahadzaryab1 added 4 commits October 8, 2024 21:43

Implement ToSever For HTTP

c65a964

Signed-off-by: Mahad Zaryab <[email protected]>

Implement ToSever For GRPC

23ec8c8

Signed-off-by: Mahad Zaryab <[email protected]>

Fix Unit Test

b793db0

Signed-off-by: Mahad Zaryab <[email protected]>

Move Recovery Handler To Common

1b71c46

Signed-off-by: Mahad Zaryab <[email protected]>

Make Function Private

0a7d837

Signed-off-by: Mahad Zaryab <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] [query] use otel's implementation for constructing http and grpc servers #6055

[WIP] [query] use otel's implementation for constructing http and grpc servers #6055

mahadzaryab1 commented Oct 5, 2024

codecov bot commented Oct 5, 2024 •

edited

Loading

mahadzaryab1 commented Oct 5, 2024

yurishkuro commented Oct 5, 2024

mahadzaryab1 commented Oct 5, 2024

mahadzaryab1 commented Oct 5, 2024

yurishkuro Oct 6, 2024

yurishkuro Oct 6, 2024

yurishkuro Oct 6, 2024

yurishkuro Oct 6, 2024

mahadzaryab1 Oct 6, 2024

yurishkuro Oct 6, 2024

mahadzaryab1 Oct 6, 2024

yurishkuro Oct 6, 2024 •

edited

Loading

mahadzaryab1 commented Oct 9, 2024

	telset.Logger.Error("using the same port for gRPC and HTTP is deprecated; please use dedicated host ports intead")
	telset.Logger.Warning("using the same port for gRPC and HTTP is deprecated; please use dedicated ports instead")

[WIP] [query] use otel's implementation for constructing http and grpc servers #6055

Are you sure you want to change the base?

[WIP] [query] use otel's implementation for constructing http and grpc servers #6055

Conversation

mahadzaryab1 commented Oct 5, 2024

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Oct 5, 2024 • edited Loading

Codecov Report

mahadzaryab1 commented Oct 5, 2024

yurishkuro commented Oct 5, 2024

mahadzaryab1 commented Oct 5, 2024

mahadzaryab1 commented Oct 5, 2024

yurishkuro Oct 6, 2024

Choose a reason for hiding this comment

yurishkuro Oct 6, 2024

Choose a reason for hiding this comment

yurishkuro Oct 6, 2024

Choose a reason for hiding this comment

yurishkuro Oct 6, 2024

Choose a reason for hiding this comment

mahadzaryab1 Oct 6, 2024

Choose a reason for hiding this comment

yurishkuro Oct 6, 2024

Choose a reason for hiding this comment

mahadzaryab1 Oct 6, 2024

Choose a reason for hiding this comment

yurishkuro Oct 6, 2024 • edited Loading

Choose a reason for hiding this comment

mahadzaryab1 commented Oct 9, 2024

codecov bot commented Oct 5, 2024 •

edited

Loading

yurishkuro Oct 6, 2024 •

edited

Loading