[es cit] Use storage factory instead of manual creation

[Wise-Wizard]

Which problem is this PR solving?

This PR addresses a new found issue which is a part of the issue #5203 .
It initializes ES Factory in the Integration Test Suite.

Description:
This PR introduces changes to the integration tests by initializing an ElasticSearch factory (es.Factory) and utilizing it for the creation of span readers and writers. Previously, the code instantiated a new span reader/writer from the SpanStore directly, which has been replaced with the more appropriate approach of leveraging the ElasticSearch factory.

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]

Hi @yurishkuro, Can you please guide me on how to test these Github Workflows locally? I have to push everytime I make changes to see if it is right or not.

[yurishkuro]

You can check the code for the workflows in .github dir. usually we keep them very simple, all of the test business logic is in the script that the workflow runs, precisely to allow local debugging. You can run the script manually.

[Wise-Wizard]

You can check the code for the workflows in .github dir. usually we keep them very simple, all of the test business logic is in the script that the workflow runs, precisely to allow local debugging. You can run the script manually.

Understood Thanks!
Since, I am using WSL I will have to install "act" and run a specific job using act -j right? Or, is there a simpler way?

[yurishkuro]

don't know what act is - you should only need Docker

[Wise-Wizard]

don't know what act is - you should only need Docker

While googling how to run github actions locally, almost 10-12 sites used a github repo nektos/act to run the actions locally. I already am using docker, will look into it further. Thanks for your guidance.

[yurishkuro]

you don not need to run actions locally. Actions just call a shell script, you can run the shell script locally. This is the only line relevant to running ES test locally:

jaeger/.github/workflows/ci-elasticsearch.yml

Line 61 in bae96f7

run: bash scripts/es-integration-test.sh ${{ matrix.version.distribution }} ${{ matrix.version.image }}

[yurishkuro]

please rebase, there were changes in the signatures

[Wise-Wizard]

you don not need to run actions locally. Actions just call a shell script, you can run the shell script locally. This is the only line relevant to running ES test locally:

jaeger/.github/workflows/ci-elasticsearch.yml

Line 61 in bae96f7

run: bash scripts/es-integration-test.sh ${{ matrix.version.distribution }} ${{ matrix.version.image }}

Got it, Thanks a lot!

[Wise-Wizard]

if !f.archiveConfig.Enabled {
return nil, nil
}
Because of this flag, the ArchiveSpanReader is being initialized as nil.
Can you please guide me on how to Enable this value as true while initializing ES factory? I have InitFromViper but it is still not enabled

[yurishkuro]

you need to rebase from main, your branch is stale

[Wise-Wizard]

Hi, Took some time to debug the issue, but according to me the client being intialized is the reason. The client used in Factory initialization differs from the actual initialization of client, specific to the tests which is causing the Errors.
PTAL, would really appreciate your insights.

[yurishkuro]

could you please document what debugging you did and what you found? Were you able to log the queries?

[Wise-Wizard]

could you please document what debugging you did and what you found? Were you able to log the queries?

Yes, I was able to log the queries using SetTraceLog, but the logs didn't show anything evident enough for debugging.
So, instead I ran a debugger in the Main code and my Branch code.
The debugger showed the only difference in the initialisation of SpanReader...
The difference was the client passed to SpanReader which was causing the Error

[yurishkuro]

so what's the state now, were you able to resolve the issue? I see the CI is still failing.

[Wise-Wizard]

so what's the state now, were you able to resolve the issue? I see the CI is still failing.

Yes, the GetOperation and the first 2 tests were successful, but I had to do something unorthodox i.e to change the Factory file of ES and do things differently. Can you please look at it and see if it's acceptable?
Update: All the tests are successful now!

[codecov]

Codecov Report

Attention: Patch coverage is 75.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 95.25%. Comparing base (7fcbca8) to head (2863e8d).

Files	Patch %	Lines
plugin/storage/integration/integration.go	75.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5313      +/-   ##
==========================================
+ Coverage   95.16%   95.25%   +0.09%     
==========================================
  Files         340      340              
  Lines       16666    16672       +6     
==========================================
+ Hits        15860    15881      +21     
+ Misses        616      602      -14     
+ Partials      190      189       -1     

Flag	Coverage Δ
badger	11.18% <0.00%> (-1.08%)	⬇️
cassandra-3.x	19.33% <75.00%> (-1.81%)	⬇️
cassandra-4.x	19.33% <75.00%> (-1.81%)	⬇️
elasticsearch-5.x	22.20% <75.00%> (+4.50%)	⬆️
elasticsearch-6.x	22.20% <75.00%> (+4.50%)	⬆️
elasticsearch-7.x	22.25% <75.00%> (+4.50%)	⬆️
elasticsearch-8.x	22.46% <75.00%> (+4.62%)	⬆️
grpc	11.57% <0.00%> (+0.01%)	⬆️
kafka	10.81% <0.00%> (-1.04%)	⬇️
opensearch-1.x	22.31% <75.00%> (+4.54%)	⬆️
opensearch-2.x	22.31% <75.00%> (+4.56%)	⬆️
unittests	92.31% <0.00%> (-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.

[yurishkuro]

Please remove commented out code

[Wise-Wizard]

Please remove commented out code

Done, PTAL!

[yurishkuro]

I removed custom initialization in favor of CLI-based factory creation. There is room for more similar clean-up for sampling store, but it also has a bug that needs to be fixed #5333

[Wise-Wizard]

I removed custom initialization in favor of CLI-based factory creation. There is room for more similar clean-up for sampling store, but it also has a bug that needs to be fixed #5333

Sure!
Should I look into that next?