Skip to content

Conversation

@Manik2708
Copy link
Contributor

Which problem is this PR solving?

Description of the changes

  • Refactor the Factory of V1 to make it reusable for v2

How was this change tested?

  • Unit And Integration Tests

Checklist

Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
@Manik2708 Manik2708 requested a review from a team as a code owner April 30, 2025 17:36
@Manik2708 Manik2708 requested a review from pavolloffay April 30, 2025 17:37
@Manik2708
Copy link
Contributor Author

This PR is for getting a direction, will complete the code-coverage once the approach is finalized

@codecov
Copy link

codecov bot commented Apr 30, 2025

Codecov Report

Attention: Patch coverage is 95.19231% with 10 lines in your changes missing coverage. Please review.

Project coverage is 96.21%. Comparing base (5877787) to head (c0dcfbd).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/storage/v1/elasticsearch/factory.go 92.10% 4 Missing and 2 partials ⚠️
internal/storage/v1/elasticsearch/helper.go 86.66% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7086      +/-   ##
==========================================
- Coverage   96.24%   96.21%   -0.04%     
==========================================
  Files         358      361       +3     
  Lines       21689    21748      +59     
==========================================
+ Hits        20875    20924      +49     
- Misses        609      616       +7     
- Partials      205      208       +3     
Flag Coverage Δ
badger_v1 9.86% <0.00%> (-0.04%) ⬇️
badger_v2 2.04% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v1-manual 14.84% <0.00%> (-0.06%) ⬇️
cassandra-4.x-v2-auto 2.03% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v2-manual 2.03% <0.00%> (-0.01%) ⬇️
cassandra-5.x-v1-manual 14.84% <0.00%> (-0.06%) ⬇️
cassandra-5.x-v2-auto 2.03% <0.00%> (-0.01%) ⬇️
cassandra-5.x-v2-manual 2.03% <0.00%> (-0.01%) ⬇️
elasticsearch-6.x-v1 20.67% <57.06%> (+0.43%) ⬆️
elasticsearch-7.x-v1 20.75% <57.06%> (+0.43%) ⬆️
elasticsearch-8.x-v1 20.93% <57.06%> (+0.43%) ⬆️
elasticsearch-8.x-v2 2.04% <0.00%> (-0.01%) ⬇️
grpc_v1 11.40% <0.00%> (-0.05%) ⬇️
grpc_v2 2.04% <0.00%> (-0.01%) ⬇️
kafka-3.x-v1 10.14% <0.00%> (-0.04%) ⬇️
kafka-3.x-v2 2.04% <0.00%> (-0.01%) ⬇️
memory_v2 2.04% <0.00%> (-0.01%) ⬇️
opensearch-1.x-v1 20.80% <57.06%> (+0.43%) ⬆️
opensearch-2.x-v1 20.80% <57.06%> (+0.43%) ⬆️
opensearch-2.x-v2 2.04% <0.00%> (-0.01%) ⬇️
query 2.04% <0.00%> (-0.01%) ⬇️
tailsampling-processor 0.55% <0.00%> (-0.01%) ⬇️
unittests 95.02% <92.78%> (-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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Manik2708 Manik2708 requested a review from yurishkuro May 6, 2025 14:30
@Manik2708 Manik2708 marked this pull request as draft May 8, 2025 04:49
Manik2708 added 4 commits May 8, 2025 10:37
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
@Manik2708 Manik2708 marked this pull request as ready for review May 9, 2025 10:14
@Manik2708
Copy link
Contributor Author

@yurishkuro I have some questions and problems:

  1. CI is green (integration tests of ES/OS for v2 are passing), does that mean the v2 factory is alright?
  2. I can't get the reason why build binaries is failing, can you help here?
  3. There are some problems I am getting while completing the code-coverage: How to cover the mapping builder error and NewFactoryWithConfig as it initalizes the client internally so we can't mock the client.

Signed-off-by: Manik2708 <[email protected]>
@Manik2708 Manik2708 requested a review from yurishkuro May 10, 2025 07:37
@Manik2708 Manik2708 marked this pull request as draft May 19, 2025 13:55
@Manik2708 Manik2708 marked this pull request as ready for review May 19, 2025 14:57
Signed-off-by: Manik2708 <[email protected]>
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.

almost ready

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

#7086 (comment) Thanks for this! I now got reminded that we can use CoreSpanWriter in the tests which is also shared by both v1 and v2

Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
@yurishkuro yurishkuro enabled auto-merge May 20, 2025 19:32
@yurishkuro yurishkuro added this pull request to the merge queue May 20, 2025
Merged via the queue into jaegertracing:main with commit 9ccbdb1 May 20, 2025
59 checks passed
@yurishkuro
Copy link
Member

@Manik2708 congrats! this was a difficult PR, any lessons learned how this could've gone smoother / faster?

@Manik2708
Copy link
Contributor Author

@Manik2708 congrats! this was a difficult PR, any lessons learned how this could've gone smoother / faster?

@yurishkuro Thanks! I learnt so many lessons. 3 months before I didn't have any experience in writing good code but now I can say I have a bit. The biggest lesson I learnt was that I need to think from the reviewer point of view also. I think that was the reason why sometimes things went slow! Sometimes changes were large and sometimes unnecessary. So will try to review the PRs in future of other contributors so that while writing code I visualize the needs of reviewer so as to make the job of reviewer easy. Thanks a lot for your patience!

@Manik2708 Manik2708 deleted the esfactoryv2 branch May 21, 2025 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/storage changelog:refactoring Internal code refactoring without functional changes storage/elasticsearch v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants