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

(batchprocessor): Move single shard batcher creation to the constructor #11594

Merged

Conversation

wildum
Copy link
Contributor

@wildum wildum commented Nov 4, 2024

Description

The single-shard batcher is currently created and started in the start function. If the consume function is called before the start function, sb.single will be nil and it will panic. I'm not sure if that can happen in the Otel collector but it might happen in distributions that have a different runtime (such as Alloy - Grafana's distribution).

I think that conceptually, it's better to create the shard in the constructor and start it in the start function.

PS: I did not create an entry in the changelog because the initial PR for this change didn't but let me know if I should add one.

Credits also to @thampiotr for this fix

@wildum wildum requested a review from a team as a code owner November 4, 2024 14:50
@wildum wildum requested a review from codeboten November 4, 2024 14:50
Copy link

codecov bot commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.57%. Comparing base (0982ff8) to head (c2e6ec3).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #11594   +/-   ##
=======================================
  Coverage   91.57%   91.57%           
=======================================
  Files         441      441           
  Lines       23873    23873           
=======================================
  Hits        21861    21861           
  Misses       1640     1640           
  Partials      372      372           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bogdandrutu
Copy link
Member

I'm not sure if that can happen in the Otel collector but it might happen in distributions that have a different runtime (such as Alloy - Grafana's distribution).

In the collector it cannot and Grafana uses the components wrong /cc @jpkrohling. The contract says here.

Another problem is that Alloy is not a otel distribution if they don't respect collectors contracts. cc @jpkrohling

FYI: I am ok with this change, but if in the future we break others because they do not respect the contract that is not considered a breaking change and fixes may not be approved/accepted.

@bogdandrutu
Copy link
Member

@wildum please add a changelog.

@thampiotr
Copy link

In the collector it cannot and Grafana uses the components wrong /cc @jpkrohling. The contract says here.

You're right, we shouldn't depend on the implementation detail like this. We're working on a fix for this in Alloy and should be able to handle OTel component's lifecycle correctly. cc @jpkrohling

@wildum
Copy link
Contributor Author

wildum commented Nov 4, 2024

@wildum please add a changelog.

Done. I put 'enhancement' because it's not a bug for the collector. Let me know if you prefer 'bug_fix'.

In the collector it cannot and Grafana uses the components wrong /cc @jpkrohling. The contract says here.
Another problem is that Alloy is not a otel distribution if they don't respect collectors contracts. cc @jpkrohling
FYI: I am ok with this change, but if in the future we break others because they do not respect the contract that is not considered a breaking change and fixes may not be approved/accepted.

Thanks for pointing this out. We noticed this today while investigating the problem we had with the batch processor. We will work towards improving Alloy to respect the documented contracts.

@bogdandrutu bogdandrutu merged commit 69e7a40 into open-telemetry:main Nov 4, 2024
46 checks passed
@github-actions github-actions bot added this to the next release milestone Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants