[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 78.57143% with 21 lines in your changes missing coverage. Please review.

Project coverage is 96.34%. Comparing base (acbc0e1) to head (3231bfd).

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6055      +/-   ##
==========================================
- Coverage   96.46%   96.34%   -0.13%     
==========================================
  Files         352      352              
  Lines       19986    20045      +59     
==========================================
+ Hits        19280    19312      +32     
- Misses        522      544      +22     
- Partials      184      189       +5

Flag	Coverage Δ
badger_v1	`8.42% <ø> (ø)`
badger_v2	`1.70% <ø> (ø)`
cassandra-4.x-v1	`14.57% <ø> (ø)`
cassandra-4.x-v2	`1.64% <ø> (ø)`
cassandra-5.x-v1	`14.57% <ø> (ø)`
cassandra-5.x-v2	`1.64% <ø> (ø)`
elasticsearch-6.x-v1	`18.72% <ø> (-0.02%)`	⬇️
elasticsearch-7.x-v1	`18.81% <ø> (+0.01%)`	⬆️
elasticsearch-8.x-v1	`18.98% <ø> (ø)`
elasticsearch-8.x-v2	`1.69% <ø> (-0.02%)`	⬇️
grpc_v1	`8.77% <ø> (ø)`
grpc_v2	`?`
kafka-v1	`8.99% <ø> (ø)`
kafka-v2	`1.70% <ø> (ø)`
memory_v2	`1.70% <ø> (ø)`
opensearch-1.x-v1	`18.86% <ø> (+0.01%)`	⬆️
opensearch-2.x-v1	`18.85% <ø> (ø)`
opensearch-2.x-v2	`1.69% <ø> (ø)`
tailsampling-processor	`0.48% <ø> (ø)`
unittests	`95.26% <78.57%> (-0.13%)`	⬇️

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 <mahadzaryab1@gmail.com>

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.

cmd/query/app/server.go

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 <mahadzaryab1@gmail.com>

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 <mahadzaryab1@gmail.com>

yurishkuro · 2024-10-19T19:28:39Z

should have 1 item(s), but has 2

It seems some unexpected spans are being created. This may not necessary be a bad thing, i.e. we may need to adjust the test to allow that. I'd recommend running the test with commented out docker kill so that you can go and inspect the trace in Jaeger UI in order to understand what exactly is being created and whether we want it.

mahadzaryab1 · 2024-10-20T16:21:37Z

should have 1 item(s), but has 2

It seems some unexpected spans are being created. This may not necessary be a bad thing, i.e. we may need to adjust the test to allow that. I'd recommend running the test with commented out docker kill so that you can go and inspect the trace in Jaeger UI in order to understand what exactly is being created and whether we want it.

@yurishkuro This is what I'm seeing in the jaeger UI. What're your thoughts?

yurishkuro · 2024-10-20T17:01:58Z

The /api/traces request, is it a GET or a POST? Get is ok to trace, post is not.

mahadzaryab1 · 2024-10-20T17:26:22Z

The /api/traces request, is it a GET or a POST? Get is ok to trace, post is not.

@yurishkuro It looks to be a GET request.

yurishkuro · 2024-10-20T18:45:28Z

Ok, then how is it different from how the test worked before your change? Did we not have tracing on that endpoint?

mahadzaryab1 · 2024-10-20T18:52:02Z

@yurishkuro It looks like the second trace that's the one that is new

yurishkuro · 2024-10-20T21:11:23Z

I think we should improve selectivity of the test - it should not be breaking if some extra trace is created, its objective is to check that the trace it needs is created.

mahadzaryab1 added 2 commits October 5, 2024 18:12

Pass Context To Server Functions

af14b4c

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Pass Host To NewServer Function

49f8261

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

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 <mahadzaryab1@gmail.com>

Make Helper For Registering GRPC Server

8b4b4ca

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Add component.Host To telemetry.Setting

b0ee94f

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

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 <mahadzaryab1@gmail.com>

Implement ToSever For GRPC

23ec8c8

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Fix Unit Test

b793db0

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Move Recovery Handler To Common

1b71c46

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 and others added 2 commits October 9, 2024 07:46

Make Function Private

0a7d837

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Merge branch 'main' into otel-servers

e8bf982

yurishkuro added the changelog:bugfix-or-minor-feature label Oct 19, 2024

Merge branch 'main' into otel-servers

3231bfd

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

mahadzaryab1 Oct 6, 2024

yurishkuro Oct 6, 2024

mahadzaryab1 Oct 6, 2024

yurishkuro Oct 6, 2024 •

edited

Loading

mahadzaryab1 commented Oct 9, 2024

yurishkuro commented Oct 19, 2024

mahadzaryab1 commented Oct 20, 2024

yurishkuro commented Oct 20, 2024

mahadzaryab1 commented Oct 20, 2024

yurishkuro commented Oct 20, 2024

mahadzaryab1 commented Oct 20, 2024

yurishkuro commented Oct 20, 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

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

yurishkuro commented Oct 19, 2024

mahadzaryab1 commented Oct 20, 2024

yurishkuro commented Oct 20, 2024

mahadzaryab1 commented Oct 20, 2024

yurishkuro commented Oct 20, 2024

mahadzaryab1 commented Oct 20, 2024

yurishkuro commented Oct 20, 2024

codecov bot commented Oct 5, 2024 •

edited

Loading

yurishkuro Oct 6, 2024 •

edited

Loading