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

Fix unnecessary and incorrect reallocation #1041

Merged
merged 14 commits into from
Aug 22, 2022

Conversation

jaronoff97
Copy link
Contributor

Closes #1038, solves an issue where the allocator re-assigns collectors unnecessarily. This is accomplished by removing the unnecessary logic around allocating targets and collectors as they come in. Previously, the collectors were entirely cleared whenever the SetCollectors() method was called. A similar amount of confusion happened with discovered targets where the targetsWaiting map which was used for decisions in allocation would be cleared, allowing for a scenario where one target could be assigned to multiple collectors.

The new logic combines the SetWaitingTargets and processWaitingTargets methods in to one to help clarify the logic. Logic is also simplified for SetCollectors which simply reassigns all targets discovered after the changed collector list is processed.

Before this change:

Screen Shot 2022-08-18 at 1 42 51 PM

After this change:

Screen Shot 2022-08-18 at 1 43 13 PM

@jaronoff97 jaronoff97 requested a review from a team August 18, 2022 17:43
cmd/otel-allocator/allocation/allocator.go Outdated Show resolved Hide resolved
cmd/otel-allocator/allocation/allocator.go Show resolved Hide resolved
cmd/otel-allocator/allocation/allocator.go Outdated Show resolved Hide resolved
cmd/otel-allocator/allocation/allocator.go Outdated Show resolved Hide resolved
cmd/otel-allocator/allocation/allocator.go Outdated Show resolved Hide resolved
cmd/otel-allocator/allocation/allocator.go Show resolved Hide resolved
cmd/otel-allocator/allocation/allocator.go Outdated Show resolved Hide resolved
cmd/otel-allocator/allocation/allocator.go Outdated Show resolved Hide resolved
cmd/otel-allocator/allocation/allocator.go Show resolved Hide resolved
cmd/otel-allocator/allocation/allocator.go Show resolved Hide resolved
@jaronoff97 jaronoff97 requested review from Aneurysm9, kristinapathak and secustor and removed request for Aneurysm9 and kristinapathak August 19, 2022 15:23
@jaronoff97 jaronoff97 force-pushed the fix-allocation-problems branch from a2eb58b to 31c7da3 Compare August 22, 2022 14:18
@jpkrohling
Copy link
Member

Approving by proxy (code owners approved), and merging.

@jpkrohling jpkrohling merged commit ce4b1be into open-telemetry:main Aug 22, 2022
@jaronoff97 jaronoff97 deleted the fix-allocation-problems branch August 22, 2022 16:12
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
…lemetry#1041)

* Resolve bug where TA doesn't allocate all targets

* Cleanup cleanup

* Changed how targets and collectors are allocated

* Comments

* added a test

* Fix comment spacing

* Updated based on feedback

* Metrics fix, comment

* updated from feedback

* Change where metric is recorded
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.

[target-allocator] Target allocator assigns target to multiple collectors
5 participants