Skip to content

Fix - TODO remove redundant sanitize call from queue #7443

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

Closed
wants to merge 3 commits into from

Conversation

AnmolxSingh
Copy link
Contributor

Which problem is this PR solving?

  • Resolves a TODO

Description of the changes

  • removed sanitize func being called while the spans were in queue

Checklist

Signed-off-by: anmol7344 <[email protected]>
@AnmolxSingh AnmolxSingh requested a review from a team as a code owner August 13, 2025 13:54
@AnmolxSingh AnmolxSingh changed the title Fix - TODO Fix - TODO remove redundant sanitize call from queue Aug 13, 2025
Copy link

codecov bot commented Aug 13, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.44%. Comparing base (b11cced) to head (67e1e36).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7443      +/-   ##
==========================================
- Coverage   96.44%   96.44%   -0.01%     
==========================================
  Files         377      377              
  Lines       23092    23094       +2     
==========================================
+ Hits        22271    22272       +1     
- Misses        622      623       +1     
  Partials      199      199              
Flag Coverage Δ
badger_v1 9.02% <ø> (ø)
badger_v2 1.70% <ø> (ø)
cassandra-4.x-v1-manual 11.68% <ø> (ø)
cassandra-4.x-v2-auto 1.69% <ø> (ø)
cassandra-4.x-v2-manual 1.69% <ø> (ø)
cassandra-5.x-v1-manual 11.68% <ø> (ø)
cassandra-5.x-v2-auto 1.69% <ø> (ø)
cassandra-5.x-v2-manual 1.69% <ø> (ø)
elasticsearch-6.x-v1 ?
elasticsearch-7.x-v1 16.62% <ø> (ø)
elasticsearch-8.x-v1 16.77% <ø> (ø)
elasticsearch-8.x-v2 1.70% <ø> (ø)
elasticsearch-9.x-v2 1.70% <ø> (ø)
grpc_v1 10.22% <ø> (ø)
grpc_v2 1.70% <ø> (ø)
kafka-3.x-v1 9.67% <ø> (ø)
kafka-3.x-v2 1.70% <ø> (ø)
memory_v2 1.70% <ø> (ø)
opensearch-1.x-v1 16.67% <ø> (ø)
opensearch-2.x-v1 16.67% <ø> (ø)
opensearch-2.x-v2 1.70% <ø> (ø)
opensearch-3.x-v2 1.70% <ø> (ø)
query 1.70% <ø> (ø)
tailsampling-processor 0.47% <ø> (ø)
unittests 95.43% <100.00%> (+<0.01%) ⬆️

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.

@AnmolxSingh AnmolxSingh marked this pull request as draft August 13, 2025 14:14
Signed-off-by: anmol7344 <[email protected]>
Signed-off-by: anmol7344 <[email protected]>
@AnmolxSingh AnmolxSingh marked this pull request as ready for review August 13, 2025 18:35
Comment on lines +280 to +282
if batch.GetSpanFormat() == processor.JaegerSpanFormat {
sp.sanitizer(mSpan)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Race condition introduced by moving sanitizer call before enqueueSpan. The sanitizer modifies the span object, but the comment at line 316 explicitly states 'spans may share the Process object, so no changes should be made to Process in this function as it may cause race conditions.' The sanitizer can modify Process.Tags, creating a race condition when multiple spans share the same Process object. The original code correctly sanitized spans after dequeuing to avoid this race condition.

Suggested change
if batch.GetSpanFormat() == processor.JaegerSpanFormat {
sp.sanitizer(mSpan)
}
// The sanitizer will be applied when the span is dequeued

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this change fails the unit test TestSpanProcessorErrors in span_processor_test.go

@AnmolxSingh
Copy link
Contributor Author

@yurishkuro
Please review this pr.

@yurishkuro
Copy link
Member

collector is v1 code that has 4 month of life left. I don't want to change it.

@yurishkuro yurishkuro closed this Aug 16, 2025
@AnmolxSingh
Copy link
Contributor Author

Oh!
But where I could find the jaeger v2 collector code?

@yurishkuro
Copy link
Member

otel collector

@AnmolxSingh
Copy link
Contributor Author

AnmolxSingh commented Aug 16, 2025

will ingester remain a separate binary in Jaeger v2 or will be folded into otel collector?

@yurishkuro
Copy link
Member

V2 is a single binary that can be deployed in multiple roles

@AnmolxSingh AnmolxSingh deleted the todo branch August 16, 2025 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants