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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 21 commits into from
Dec 11, 2024
Merged

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

merged 21 commits into from
Dec 11, 2024

Conversation

chahatsagarmain
Copy link
Contributor

@chahatsagarmain chahatsagarmain commented Nov 29, 2024 •
edited
Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

@chahatsagarmain
use otel helper in collector
641ddf2 
Signed-off-by: chahatsagarmain <[email protected]>
@chahatsagarmain chahatsagarmain requested a review from a team as a code owner November 29, 2024 16:51
@codecov Codecov
Copy link

codecov bot commented Nov 29, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 86.76471% with 9 lines in your changes missing coverage. Please review.

Project coverage is 96.05%. Comparing base (53a8053) to head (1b2ea5d).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/telemetry/settings.go 0.00% 6 Missing ⚠️
cmd/collector/app/server/grpc.go 75.00% 1 Missing and 1 partial ⚠️
cmd/collector/app/collector.go 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6277      +/-   ##
==========================================
- Coverage   96.17%   96.05%   -0.12%     
==========================================
  Files         357      357              
  Lines       20553    20473      -80     
==========================================
- Hits        19766    19666     -100     
- Misses        601      617      +16     
- Partials      186      190       +4
Flag Coverage Δ
badger_v1 8.76% <0.00%> (-0.01%) ⬇️
badger_v2 1.60% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v1-manual 14.73% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v2-auto 1.55% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v2-manual 1.55% <0.00%> (-0.01%) ⬇️
cassandra-5.x-v1-manual 14.73% <0.00%> (-0.01%) ⬇️
cassandra-5.x-v2-auto 1.55% <0.00%> (-0.01%) ⬇️
cassandra-5.x-v2-manual 1.55% <0.00%> (-0.01%) ⬇️
elasticsearch-6.x-v1 18.36% <0.00%> (-0.02%) ⬇️
elasticsearch-7.x-v1 18.44% <0.00%> (-0.01%) ⬇️
elasticsearch-8.x-v1 18.60% <0.00%> (-0.01%) ⬇️
elasticsearch-8.x-v2 1.60% <0.00%> (+<0.01%) ⬆️
grpc_v1 10.24% <0.00%> (-0.01%) ⬇️
grpc_v2 7.79% <0.00%> (-0.01%) ⬇️
kafka-v1 9.16% <0.00%> (-0.01%) ⬇️
kafka-v2 1.60% <0.00%> (-0.01%) ⬇️
memory_v2 1.60% <0.00%> (-0.01%) ⬇️
opensearch-1.x-v1 18.47% <0.00%> (-0.02%) ⬇️
opensearch-2.x-v1 18.47% <0.00%> (-0.02%) ⬇️
opensearch-2.x-v2 1.60% <0.00%> (-0.01%) ⬇️
tailsampling-processor 0.45% <0.00%> (-0.01%) ⬇️
unittests 94.96% <86.76%> (-0.12%) ⬇️

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.

@chahatsagarmain
migrate to otel config
4bf8eaa 
Signed-off-by: chahatsagarmain <[email protected]>
@chahatsagarmain chahatsagarmain marked this pull request as draft November 29, 2024 21:05
@chahatsagarmain chahatsagarmain changed the title [collector] Use OTEL helper in Collector [WIP][collector] Use OTEL helper in Collector Nov 29, 2024
@chahatsagarmain
Copy link
Contributor Author

@yurishkuro how do i handle tenancy for GRPC after utilzing configgrpc ?

@chahatsagarmain chahatsagarmain changed the title [WIP][collector] Use OTEL helper in Collector [WIP][collector] Switch to OTEL's http/grpc server Nov 29, 2024
@yurishkuro
Copy link
Member

how do i handle tenancy for GRPC after utilzing configgrpc ?

See examples in #6121 and #6055. There are standard inteceptors for tenancy that we add to both http and grpc.

@chahatsagarmain
deletion of unused functions
31588ef 
Signed-off-by: chahatsagarmain <[email protected]>
@chahatsagarmain
tenancy and fixes
624bb69 
Signed-off-by: chahatsagarmain <[email protected]>
@chahatsagarmain
Copy link
Contributor Author

chahatsagarmain commented Nov 30, 2024 •
edited
Loading

jaeger/cmd/collector/app/handler/otlp_receiver.go

Line 63 in 624bb69

fmt.Printf("OTLP HTTP Endpoint: %s\n", otlpReceiverConfig.HTTP.ServerConfig.Endpoint)

@yurishkuro the endpoint is being set correctly in test OTLP HTTP Endpoint: :0 OTLP GRPC Endpoint: :0 but the its throwing an error could not start OTLP receiver: could not start the OTLP receiver: listen: unknown network

mahadzaryab1
mahadzaryab1 reviewed Dec 7, 2024
View reviewed changes
Copy link
Collaborator

@mahadzaryab1 mahadzaryab1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few more comments

cmd/collector/app/collector.go Outdated Show resolved Hide resolved
cmd/collector/app/collector_test.go Outdated Show resolved Hide resolved
cmd/collector/app/handler/otlp_receiver_test.go
GRPC configgrpc.ServerConfig
HTTP confighttp.ServerConfig
}{
Enabled: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we setting Enabled to true?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's part of the main config and is exposed as a CLI flag today (on by default but can be disabled by the user). So I assume it does need to be set to true manually here if we're not initalizing this struct by other means.

cmd/collector/app/handler/zipkin_receiver_tls_test.go Outdated Show resolved Hide resolved
cmd/collector/app/handler/zipkin_receiver_tls_test.go Outdated Show resolved Hide resolved
cmd/collector/app/server/grpc.go Outdated Show resolved Hide resolved
pkg/config/corscfg/options.go Outdated Show resolved Hide resolved
@chahatsagarmain
resolved comments
f36c84d 
Signed-off-by: chahatsagarmain <[email protected]>
@chahatsagarmain
missing test case
71194a5 
Signed-off-by: chahatsagarmain <[email protected]>
chahatsagarmain and others added 3 commits December 9, 2024 19:56
@chahatsagarmain @yurishkuro
Update cmd/collector/app/server/http.go
06cc1d3 
Co-authored-by: Yuri Shkuro <[email protected]>
Signed-off-by: chahat sagar <[email protected]>
@chahatsagarmain
changes
6bfb708 
Signed-off-by: chahatsagarmain <[email protected]>
@chahatsagarmain
Merge branch 'collector-tlscfg' of https://github.com/chahatsagarmain…
772a384 
…/jaeger into collector-tlscfg

Signed-off-by: chahatsagarmain <[email protected]>
@chahatsagarmain
reviewed changes
5e69010 
Signed-off-by: chahatsagarmain <[email protected]>
@chahatsagarmain
simplified code
6943164 
Signed-off-by: chahatsagarmain <[email protected]>
yurishkuro
yurishkuro reviewed Dec 11, 2024
View reviewed changes
cmd/collector/app/flags/flags.go
// CORS allows CORS requests , sets the values for Allowed Headers and Allowed Origins.
CORS corscfg.Options
// KeepAlive configures allow Keep-Alive for Zipkin HTTP server
confighttp.ServerConfig
KeepAlive bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is unused probably since migrating to Zipkin receiver.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#6341

@chahatsagarmain chahatsagarmain marked this pull request as ready for review December 11, 2024 22:53
@yurishkuro yurishkuro enabled auto-merge (squash) December 11, 2024 23:00
@yurishkuro
Copy link
Member

Thanks!

In the future let's try to go with smaller PRs.

@yurishkuro yurishkuro disabled auto-merge December 11, 2024 23:06
@yurishkuro yurishkuro merged commit 0f2d50a into jaegertracing:main Dec 11, 2024
53 of 54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@mahadzaryab1 mahadzaryab1 Awaiting requested review from mahadzaryab1

Assignees
No one assigned
Labels
changelog:bugfix-or-minor-feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[collector] Switch to use OTEL's http/grpc servers
3 participants
@chahatsagarmain @yurishkuro @mahadzaryab1