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

Implement Telemetery struct for V1 Components Initialization #5695

Merged
merged 35 commits into from
Jul 9, 2024

Conversation

Wise-Wizard
Copy link
Contributor

Which problem is this PR solving?

This PR addresses a part of the issue #5633

Description of the changes
This is a Draft PR to achieve Observability Parity between V1 and V2 components by creating an unified telemetery container to pass observability clients to V1 components.
How was this change tested?

The changes were tested by running the following command:

make test

Checklist

  • I have read 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

@Wise-Wizard Wise-Wizard requested a review from a team as a code owner July 2, 2024 17:43
@Wise-Wizard Wise-Wizard requested a review from yurishkuro July 2, 2024 17:43
@Wise-Wizard Wise-Wizard marked this pull request as draft July 2, 2024 17:43
@dosubot dosubot bot added area/otel changelog:new-feature Change that should be called out as new feature in CHANGELOG changelog:test Change that's adding missing tests or correcting existing tests labels Jul 2, 2024
Signed-off-by: Wise-Wizard <[email protected]>
Copy link

codecov bot commented Jul 2, 2024

Codecov Report

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

Project coverage is 96.84%. Comparing base (295293c) to head (25f8d10).

Files Patch % Lines
cmd/query/app/server.go 77.50% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5695      +/-   ##
==========================================
- Coverage   96.88%   96.84%   -0.05%     
==========================================
  Files         334      335       +1     
  Lines       16141    16155      +14     
==========================================
+ Hits        15639    15645       +6     
- Misses        333      340       +7     
- Partials      169      170       +1     
Flag Coverage Δ
badger_v1 8.06% <ø> (ø)
badger_v2 1.90% <ø> (ø)
cassandra-3.x-v1 16.62% <ø> (ø)
cassandra-3.x-v2 1.82% <ø> (ø)
cassandra-4.x-v1 16.62% <ø> (ø)
cassandra-4.x-v2 1.82% <ø> (ø)
elasticsearch-6.x-v1 18.79% <ø> (-0.02%) ⬇️
elasticsearch-7.x-v1 18.86% <ø> (ø)
elasticsearch-8.x-v1 19.05% <ø> (+0.01%) ⬆️
elasticsearch-8.x-v2 1.90% <ø> (+0.01%) ⬆️
grpc_v1 9.46% <ø> (+0.01%) ⬆️
grpc_v2 7.41% <ø> (ø)
kafka 9.75% <ø> (ø)
opensearch-1.x-v1 18.91% <ø> (ø)
opensearch-2.x-v1 18.91% <ø> (+0.01%) ⬆️
opensearch-2.x-v2 1.90% <ø> (ø)
unittests 95.23% <86.76%> (-0.05%) ⬇️

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.

@Wise-Wizard
Copy link
Contributor Author

The initialization of the Telemetery Struct can't be done from the source itself, because different components have different requirements. Some mains() are passing jTracer.NoOp, whereas some aren't passing any Tracer/Metrics Factory itself.
So, the telemetery struct will have different initialization every time.
Am I right?

cmd/all-in-one/main.go Outdated Show resolved Hide resolved
cmd/query/app/server.go Outdated Show resolved Hide resolved
Wise-Wizard and others added 4 commits July 3, 2024 09:32
Co-authored-by: Yuri Shkuro <[email protected]>
Signed-off-by: Saransh Shankar <[email protected]>
Signed-off-by: Wise-Wizard <[email protected]>
Signed-off-by: Wise-Wizard <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
Signed-off-by: Saransh Shankar <[email protected]>
cmd/all-in-one/main.go Outdated Show resolved Hide resolved
Signed-off-by: Wise-Wizard <[email protected]>
cmd/query/app/server.go Outdated Show resolved Hide resolved
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

lgtm, let's clean up tests / lint and merge

@Wise-Wizard
Copy link
Contributor Author

lgtm, let's clean up tests / lint and merge

What about the problem of jTracer being separately intialized in V2 context and the only field which has to be created and passed uniquely?
Screenshot 2024-07-06 003423

Signed-off-by: Wise-Wizard <[email protected]>
@Wise-Wizard
Copy link
Contributor Author

Also, Please review the server_Test.go file, should I keep initializing the telset in each function call, or should I create a separate function initializing the telset, because args for logger (zaptest.Logger/flagsvc.Logger) and healthcheck can be different based on the function that is calling it.

@yurishkuro
Copy link
Member

What about the problem of jTracer being separately intialized in V2 context

I think in the current form it's not worse than before, but yes, we need to address it later by properly instructing OTEL Collector to configure the telemetry providers.

@yurishkuro
Copy link
Member

Please review the server_Test.go file, should I keep initializing the telset in each function call, or should I create a separate function initializing the telset

create a helper function and call it from each test

Signed-off-by: Wise-Wizard <[email protected]>
Signed-off-by: Wise-Wizard <[email protected]>
Signed-off-by: Wise-Wizard <[email protected]>
@Wise-Wizard
Copy link
Contributor Author

Wise-Wizard commented Jul 6, 2024

In the TestServerStart, the line server.Shutdown is resulting in a nil pointer deference error.
I am not able to figure out the reason for it to throw an error. It is specific to the one test case which I have commented out int server_Test.go
Screenshot 2024-07-06 101633

@yurishkuro
Copy link
Member

In the TestServerStart, the line server.Shutdown is resulting in a nil pointer deference error.

why? If nil pointer results in a panic you get the exact line in the stacktrace.

Signed-off-by: Wise-Wizard <[email protected]>
@Wise-Wizard
Copy link
Contributor Author

Wise-Wizard commented Jul 7, 2024

In the TestServerStart, the line server.Shutdown is resulting in a nil pointer deference error.

why? If nil pointer results in a panic you get the exact line in the stacktrace.

Yes, Line 316 in server.go is causing an Error, gRPC go routine is cauing a nil pointer deference error.
Screenshot 2024-07-07 211311
I tried adding nil checks but not able to figure out the root cause for this error.

Signed-off-by: Wise-Wizard <[email protected]>
@yurishkuro yurishkuro marked this pull request as ready for review July 9, 2024 19:23
@yurishkuro yurishkuro added v2 changelog:exprimental Change to an experimental part of the code and removed changelog:new-feature Change that should be called out as new feature in CHANGELOG changelog:test Change that's adding missing tests or correcting existing tests labels Jul 9, 2024
@yurishkuro yurishkuro merged commit a5fd223 into jaegertracing:main Jul 9, 2024
46 of 47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/otel changelog:exprimental Change to an experimental part of the code v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants