[query] Use OTEL's helpers for grpc server #6055

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

Which problem is this PR solving?

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

Description of the changes

Migrates the GRPC query server to create the server using OTEL rather than using a custom implementation
Adds a log to warn users that having the same port for GRPC and HTTP is now deprecated. We'll be removing this functionality after Feb 2025.

How was this change tested?

Unit tests / CI

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 96.55172% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.45%. Comparing base (f9474f9) to head (98eabd5).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...md/jaeger/internal/extension/jaegerquery/server.go	66.66%	0 Missing and 1 partial ⚠️
cmd/query/app/server.go	97.29%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6055      +/-   ##
==========================================
- Coverage   96.48%   96.45%   -0.03%     
==========================================
  Files         352      352              
  Lines       19973    20007      +34     
==========================================
+ Hits        19270    19297      +27     
- Misses        520      526       +6     
- Partials      183      184       +1

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% <ø> (ø)`
elasticsearch-7.x-v1	`18.80% <ø> (ø)`
elasticsearch-8.x-v1	`18.97% <ø> (-0.02%)`	⬇️
elasticsearch-8.x-v2	`1.70% <ø> (ø)`
grpc_v1	`8.78% <ø> (ø)`
grpc_v2	`6.72% <ø> (ø)`
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.86% <ø> (ø)`
opensearch-2.x-v2	`1.70% <ø> (ø)`
tailsampling-processor	`0.48% <ø> (ø)`
unittests	`95.37% <96.55%> (-0.03%)`	⬇️

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.

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

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

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

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.

cmd/all-in-one/all_in_one_test.go

cmd/all-in-one/main.go

cmd/query/app/server.go

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

cmd/query/app/server.go

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

cmd/query/app/flags.go

cmd/query/app/server.go

cmd/query/app/server_test.go

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

## Which problem is this PR solving? - Towards #6026 ## Description of the changes - Fixes an issue from #6055 where the condition for routing to the legacy implementation was accidentally flipped. ## How was this change tested? - CI ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

This reverts commit c96790a.

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

cmd/query/app/server.go Outdated Show resolved Hide resolved

cmd/query/app/server.go Outdated Show resolved Hide resolved

cmd/query/app/server.go Outdated Show resolved Hide resolved

cmd/query/app/server.go Outdated Show resolved Hide resolved

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

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

mahadzaryab1 commented Oct 23, 2024

View reviewed changes

cmd/all-in-one/all_in_one_test.go Outdated Show resolved Hide resolved

yurishkuro reviewed Oct 23, 2024

View reviewed changes

cmd/all-in-one/main.go Outdated Show resolved Hide resolved

yurishkuro reviewed Oct 23, 2024

View reviewed changes

cmd/query/app/server.go Outdated Show resolved Hide resolved

yurishkuro reviewed Oct 23, 2024

View reviewed changes

cmd/query/app/server.go Outdated Show resolved Hide resolved

yurishkuro reviewed Oct 23, 2024

View reviewed changes

cmd/query/app/server.go Outdated Show resolved Hide resolved

mahadzaryab1 added 8 commits October 22, 2024 23:28

Pass Context To Server Functions

08b101f

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

Pass Host To NewServer Function

0fcb031

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

Make Helper For Creating HTTP Router

c26830c

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

Make Helper For Registering GRPC Server

d58826c

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

Add component.Host To telemetry.Setting

2bf6592

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

Implement ToSever For HTTP

e7e74f8

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

Implement ToSever For GRPC

dbb7544

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

Fix Unit Test

34ae635

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

yurishkuro reviewed Oct 23, 2024

View reviewed changes

cmd/query/app/server.go Show resolved Hide resolved

yurishkuro reviewed Oct 23, 2024

View reviewed changes

cmd/query/app/server.go Show resolved Hide resolved

mahadzaryab1 added 2 commits October 23, 2024 18:45

Add Log For Using Same HTTP and GRPC Ports

80e9b88

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

Use Listener From OTEL

5bc291b

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

mahadzaryab1 changed the title ~~[WIP] [query] use otel's implementation for constructing http and grpc servers~~ [query] Use OTEL's implementation for constructing grpc query server Oct 23, 2024

Merge branch 'main' into otel-servers

d019b67

mahadzaryab1 marked this pull request as ready for review October 23, 2024 22:58

mahadzaryab1 requested a review from a team as a code owner October 23, 2024 22:58

mahadzaryab1 requested a review from jkowall October 23, 2024 22:58

dosubot bot added the area/otel label Oct 23, 2024

mahadzaryab1 requested a review from yurishkuro October 23, 2024 22:59

yurishkuro reviewed Oct 23, 2024

View reviewed changes

cmd/query/app/flags.go Outdated Show resolved Hide resolved

yurishkuro reviewed Oct 23, 2024

View reviewed changes

cmd/query/app/server.go Outdated Show resolved Hide resolved

yurishkuro reviewed Oct 23, 2024

View reviewed changes

cmd/query/app/server_test.go Show resolved Hide resolved

mahadzaryab1 added 3 commits October 23, 2024 19:26

Address Feedback And Initialize GRPC Transport

f02eb84

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

Fix Unit Test

1a42fd9

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

Fix Punctuation

b004ed8

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

mahadzaryab1 requested a review from yurishkuro October 23, 2024 23:57

mahadzaryab1 added 3 commits October 23, 2024 22:05

Fix Typo

38f2fb3

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

Consolidate Defaults Between v1 And v2

291acef

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

Use DefaultOptions At Callsite

98eabd5

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

yurishkuro changed the title ~~[query] Use OTEL's implementation for constructing grpc query server~~ [query] Use OTEL's helpers for grpc server Oct 24, 2024

yurishkuro approved these changes Oct 24, 2024

View reviewed changes

yurishkuro merged commit c96790a into jaegertracing:main Oct 24, 2024
49 checks passed

mahadzaryab1 mentioned this pull request Oct 25, 2024

[fix][query] Correct query server legacy condition #6120

Merged

4 tasks

yurishkuro added a commit to yurishkuro/jaeger that referenced this pull request Nov 2, 2024

Revert "[query] Use OTEL's helpers for grpc server (jaegertracing#6055)"

cbc443c

This reverts commit c96790a.

mahadzaryab1 deleted the otel-servers branch November 23, 2024 01:32

This was referenced Nov 27, 2024

Promote mahadzaryab1 to Maintainer #6264

Closed

[WIP][collector] Switch to OTEL's http/grpc server #6277

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[query] Use OTEL's helpers for grpc server #6055

[query] Use OTEL's helpers for grpc server #6055

mahadzaryab1 commented Oct 5, 2024 •

edited

Loading

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

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

[query] Use OTEL's helpers for grpc server #6055

[query] Use OTEL's helpers for grpc server #6055

Conversation

mahadzaryab1 commented Oct 5, 2024 • edited Loading

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

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

mahadzaryab1 commented Oct 5, 2024 •

edited

Loading

codecov bot commented Oct 5, 2024 •

edited

Loading