Fix data race in ESv1 WriteSpan by using distinct inputs

[sonalgaud12]

Which problem is this PR solving?

• Resolves [Bug]: ESv1 tests fail with a data race #7168

Description of the changes

• Sent different spans to both functions to stop data race

How was this change tested?

Checklist

• I have read https://github.com/jaegertracing/jaeger/blob/master/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: npm run lint and npm run test

[sonalgaud12]

@yurishkuro I would like your review, Also please let me know if i need to write any test

[yurishkuro]

Under what conditions is the same span being modified? Storage should not be in the business of modifying input data, it has internal db model it can use to modify.

[yurishkuro]

Under what conditions is the same span being modified? Storage should not be in the business of modifying input data, it has internal db model it can use to modify.

[sonalgaud12]

To fix this, now I create a deep copy before applying any changes, so the input span remains untouched..

[yurishkuro]

You are not answering my question. The fix should be not mutating the data, not making a copy of the data and then mutating it. Copying is expensive at scale.

[sonalgaud12]

I realize that the real issue is mutating the input span, which ideally should be treated as immutable by the storage layer. Allow me some time, and I will fix it.

[sonalgaud12]

@yurishkuro Can you give me a little direction on whether I should apply transform without mutation?

[yurishkuro]

@sonalgaud12 I don't think we should be fixing the problem without understanding the root cause. First, let's find the code in storage that mutates input data. Then decide if that mutation is actually necessary or if the same capability can be achieved without it.

[Manik2708]

@sonalgaud12 Please see the comment #7168 (comment). Most probably it will fix the issue.

[yurishkuro]

This fixes the symptom, not the root cause. The storage MUST NOT mutate input data. In the context of OTEL collector framework, it's perfectly valid to configure multiple exporters for the same data, and if one of the exporters mutates this data it would still cause race conditions.

[Manik2708]

This fixes the symptom, not the root cause. The storage MUST NOT mutate input data. In the context of OTEL collector framework, it's perfectly valid to configure multiple exporters for the same data, and if one of the exporters mutates this data it would still cause race conditions.

But it isn't the model or otel span, it's the db span. Shouldn't that work?

[yurishkuro]

@Manik2708 ah, good point

[Manik2708]

Also another point, it is not related to V1, but to the base factory. I think we should now work on moving the shared code from v1 to v2 so that in future no v1 dependent code is added in v2. And it could easily be removed in future.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.17%. Comparing base (bafa507) to head (b46a236).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7196   +/-   ##
=======================================
  Coverage   96.17%   96.17%           
=======================================
  Files         366      366           
  Lines       21947    21947           
=======================================
  Hits        21107    21107           
  Misses        627      627           
  Partials      213      213           

Flag	Coverage Δ
badger_v1	9.85% <ø> (ø)
badger_v2	1.90% <ø> (ø)
cassandra-4.x-v1-manual	14.82% <ø> (ø)
cassandra-4.x-v2-auto	1.89% <ø> (ø)
cassandra-4.x-v2-manual	1.89% <ø> (ø)
cassandra-5.x-v1-manual	14.82% <ø> (ø)
cassandra-5.x-v2-auto	1.89% <ø> (ø)
cassandra-5.x-v2-manual	1.89% <ø> (ø)
elasticsearch-6.x-v1	20.65% <ø> (ø)
elasticsearch-7.x-v1	20.73% <ø> (ø)
elasticsearch-8.x-v1	20.90% <ø> (ø)
elasticsearch-8.x-v2	1.90% <ø> (ø)
grpc_v1	11.39% <ø> (ø)
grpc_v2	1.90% <ø> (ø)
kafka-3.x-v1	10.20% <ø> (ø)
kafka-3.x-v2	1.90% <ø> (ø)
memory_v2	1.90% <ø> (ø)
opensearch-1.x-v1	20.78% <ø> (ø)
opensearch-2.x-v1	20.78% <ø> (ø)
opensearch-2.x-v2	1.90% <ø> (ø)
query	1.90% <ø> (ø)
tailsampling-processor	0.51% <ø> (ø)
unittests	94.99% <ø> (ø)

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.

[sonalgaud12]

@yurishkuro Hi, all checks have passed. Could you please re-add this to the merge queue or merge it? Thanks!