Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
yurishkuro opened this issue Sep 28, 2024 · 5 comments · Fixed by #6145
Closed

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

yurishkuro opened this issue Sep 28, 2024 · 5 comments · Fixed by #6145
Labels
area/otel changelog:breaking-change Change that is breaking public APIs or established behavior good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@yurishkuro
Copy link
Member

In #6023 we changed http/grpc servers in the query service to be configured via OTEL config structs, but we still have a custom implementation for creating the servers. We should fully switch to OTEL's implementations. This will be another breaking change because in OTEL the servers cannot share the same port, which is today supported in the query service via cmux.

@yurishkuro yurishkuro added help wanted Features that maintainers are willing to accept but do not have cycles to implement good first issue Good for beginners labels Sep 28, 2024
@dosubot dosubot bot added area/otel changelog:breaking-change Change that is breaking public APIs or established behavior labels Sep 28, 2024
@Garvit-77
Copy link

Hello @yurishkuro
I would like to give it a try

@yurishkuro
Copy link
Member Author

@Garvit-77 go ahead

@mahadzaryab1
Copy link
Collaborator

@yurishkuro what should be passed in for component.Host in the ToSever method?

@yurishkuro
Copy link
Member Author

When running in v2 mode we have the real host available in the extension. For v1 it's ok to pass noop, but we need to make sure that we would still get the metrics from these endpoints as we get today.

@Garvit-77
Copy link

`type Server struct {
querySvc *querysvc.QueryService
queryOptions *QueryOptions

conn          net.Listener
grpcConn      net.Listener
httpConn      net.Listener
grpcServer    *grpc.Server
httpServer    *httpServer
separatePorts bool
bgFinished    sync.WaitGroup
telemetery.Setting

}`
As we're trying to completely switch to OTEL can't we change or remove the Server Struct ?

yurishkuro added a commit that referenced this issue Oct 24, 2024
<!--
!! Please DELETE this comment before posting.
We appreciate your contribution to the Jaeger project! 👋🎉
-->

## Which problem is this PR solving?
- Towards #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
- [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 <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
yurishkuro pushed a commit that referenced this issue Oct 25, 2024
## 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 <[email protected]>
yurishkuro pushed a commit that referenced this issue Oct 27, 2024
…#6131)

## Which problem is this PR solving?
- Towards #6026 

## Description of the changes
- Added a unit test for tracing http request endpoints in query server 

## 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/otel changelog:breaking-change Change that is breaking public APIs or established behavior good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants