[target-allocator] Add a pre-hook to the allocator to filter out dropped targets #1127

moh-osman3 · 2022-09-26T20:27:37Z

PR to resolve #1064

The goal of this PR is to drop targets before targets are assigned to the collectors by the allocator. Not necessary to allocate targets that will be dropped in the relabel step.

Now the Allocator will check if there is a Filter set and apply it to filter down the list of targets.

filterStrategy=relabel-config drops targets based on Prometheus relabel_config
No filtering is the default.

cmd/otel-allocator/prehooktargetfilter/no_op_filter.go

jaronoff97

Left a few thoughts

cmd/otel-allocator/allocation/strategy.go

cmd/otel-allocator/discovery/discovery.go

cmd/otel-allocator/prehooktargetfilter/relabel_config_filter.go

apis/v1alpha1/opentelemetrycollector_types.go

cmd/otel-allocator/prehooktargetfilter/allocator_prehook.go

cmd/otel-allocator/prehooktargetfilter/relabel_config_filter_test.go

cmd/otel-allocator/prehooktargetfilter/no_op_filter_test.go

cmd/otel-allocator/prehooktargetfilter/allocator_prehook.go

cmd/otel-allocator/prehooktargetfilter/no_op_filter.go

cmd/otel-allocator/prehooktargetfilter/relabel_config_filter.go

cmd/otel-allocator/prehooktargetfilter/allocator_prehook.go

cmd/otel-allocator/prehooktargetfilter/relabel_config_filter.go

cmd/otel-allocator/targetscommon/targets.go

cmd/otel-allocator/allocation/least_weighted.go

cmd/otel-allocator/allocation/consistent_hashing.go

cmd/otel-allocator/prehook/relabel.go

cmd/otel-allocator/prehook/relabel_test.go

Aneurysm9

I think this is definitely moving in the right direction. A few comments and questions.

cmd/otel-allocator/allocation/consistent_hashing.go

cmd/otel-allocator/allocation/least_weighted.go

Aneurysm9 · 2022-10-19T20:42:33Z

cmd/otel-allocator/allocation/consistent_hashing_test.go

+func (hook mockHook) SetConfig(map[string][]*relabel.Config) {
+}
+
+func (hook mockHook) GetConfig() map[string][]*relabel.Config {
+	dummyRet := map[string][]*relabel.Config{}
+	return dummyRet
+}


These don't seem to be at all relevant to the use of the prehook.Hook type here. Can that interface have just the Apply() method and leave [SG]etConfig() to the concrete implementation of the relabeling hook?

Hmm I'm still unsure the best way to implement this suggestion. I agree that [SG}etConfig() methods are specific to relabeling filter but therelabelCfg gets set in discovery. So I'm actually unsure if I'm able to get access to the concrete struct's method, when I only pass in the allocator interface.

cmd/otel-allocator/prehook/relabel.go

jaronoff97

Found one potential problem, otherwise this looks great!

cmd/otel-allocator/prehook/relabel.go

cmd/otel-allocator/prehook/prehook.go

kristinapathak

Left a few suggestions for having smaller, more specific interfaces with their caller code. In general, we could define smaller interfaces in the prehook package but I'm not sure how useful that would be. I would rather define smaller interfaces in the packages that use them, especially if the prehook package must define an interface with all functions in order to allow for the injection of custom hooks.

I thought these were some interesting reads:
https://stackoverflow.com/questions/73954228/in-golang-how-can-a-consumer-define-an-interface-for-a-function-that-accepts-an
https://dave.cheney.net/practical-go/presentations/gophercon-israel.html#_let_callers_define_the_interface_they_require

I also expanded on my example playground code just for fun 🙂 https://go.dev/play/p/fQU_JdouqT-

cmd/otel-allocator/discovery/discovery.go

cmd/otel-allocator/allocation/strategy.go

cmd/otel-allocator/allocation/least_weighted.go

cmd/otel-allocator/allocation/consistent_hashing.go

cmd/otel-allocator/allocation/least_weighted.go

cmd/otel-allocator/allocation/strategy.go

Aneurysm9 · 2022-10-31T20:58:09Z

cmd/otel-allocator/discovery/discovery.go

+	SetConfig(map[string][]*relabel.Config)
+}
+
+func NewManager(log logr.Logger, ctx context.Context, logger log.Logger, hook discoveryHook, options ...func(*discovery.Manager)) *Manager {


This function signature needs some love, but that can probably be handled in a separate issue.

Made a note to create an issue to refactor this function signature.

cmd/otel-allocator/prehook/prehook.go

cmd/otel-allocator/allocation/least_weighted.go

…ped targets (open-telemetry#1127) * Adding prehook allocator filter to reduce assigned targets * add metrics to track number of targets kept after filtering * add smaller interfaces local to packages using them * address review feedback * remove outdated references

kristinapathak reviewed Sep 28, 2022

View reviewed changes

cmd/otel-allocator/prehooktargetfilter/no_op_filter.go Outdated Show resolved Hide resolved

cmd/otel-allocator/prehooktargetfilter/no_op_filter.go Outdated Show resolved Hide resolved

kristinapathak mentioned this pull request Sep 30, 2022

REQUEST: New membership for kristinapathak open-telemetry/community#1213

Closed

6 tasks

moh-osman3 force-pushed the main branch 3 times, most recently from dac6b9a to cbc48bf Compare October 4, 2022 19:16

moh-osman3 marked this pull request as ready for review October 4, 2022 19:34

moh-osman3 requested a review from a team October 4, 2022 19:34

jaronoff97 reviewed Oct 4, 2022

View reviewed changes

secustor requested changes Oct 4, 2022

View reviewed changes

Aneurysm9 reviewed Oct 4, 2022

View reviewed changes

kristinapathak reviewed Oct 14, 2022

View reviewed changes

moh-osman3 force-pushed the main branch from 16d733b to 52098bc Compare October 19, 2022 07:12

Aneurysm9 reviewed Oct 19, 2022

View reviewed changes

moh-osman3 requested a review from a team October 25, 2022 00:48

jaronoff97 requested changes Oct 25, 2022

View reviewed changes

cmd/otel-allocator/prehook/relabel.go Show resolved Hide resolved

cmd/otel-allocator/prehook/prehook.go Outdated Show resolved Hide resolved

kristinapathak reviewed Oct 26, 2022

View reviewed changes

cmd/otel-allocator/discovery/discovery.go Outdated Show resolved Hide resolved

cmd/otel-allocator/allocation/strategy.go Outdated Show resolved Hide resolved

moh-osman3 force-pushed the main branch 2 times, most recently from 52d2daa to a2b20ef Compare October 27, 2022 05:37

moh-osman3 requested review from kristinapathak, jaronoff97 and Aneurysm9 and removed request for kristinapathak and jaronoff97 October 27, 2022 05:46

secustor self-requested a review October 31, 2022 19:19

Aneurysm9 reviewed Oct 31, 2022

View reviewed changes

moh-osman3 mentioned this pull request Oct 31, 2022

[target-allocator] create new target package #1213

Closed

moh-osman3 force-pushed the main branch 2 times, most recently from dd1281c to 112500e Compare November 2, 2022 09:04

moh-osman3 requested review from Aneurysm9 and secustor and removed request for secustor and Aneurysm9 November 2, 2022 09:37

secustor approved these changes Nov 2, 2022

View reviewed changes

Aneurysm9 approved these changes Nov 2, 2022

View reviewed changes

jaronoff97 approved these changes Nov 2, 2022

View reviewed changes

moh-osman3 added 5 commits November 2, 2022 15:58

Adding prehook allocator filter to reduce assigned targets

6f96fc6

add metrics to track number of targets kept after filtering

c656b86

add smaller interfaces local to packages using them

dfd5540

address review feedback

e4be134

remove outdated references

632afe7

moh-osman3 force-pushed the main branch from 98033e5 to 632afe7 Compare November 2, 2022 19:59

pavolloffay approved these changes Nov 4, 2022

View reviewed changes

pavolloffay merged commit 854eb67 into open-telemetry:main Nov 4, 2022

moh-osman3 mentioned this pull request Nov 8, 2022

[target-allocator] targets get dropped when using relabel-config Filter with serviceMonitor #1231

Closed

moh-osman3 mentioned this pull request Aug 15, 2023

REQUEST: New membership for moh-osman3 open-telemetry/community#1647

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[target-allocator] Add a pre-hook to the allocator to filter out dropped targets #1127

[target-allocator] Add a pre-hook to the allocator to filter out dropped targets #1127

moh-osman3 commented Sep 26, 2022 •

edited

Loading

jaronoff97 left a comment

Aneurysm9 left a comment

Aneurysm9 Oct 19, 2022

moh-osman3 Oct 25, 2022

jaronoff97 left a comment

kristinapathak left a comment

Aneurysm9 Oct 31, 2022

moh-osman3 Oct 31, 2022

[target-allocator] Add a pre-hook to the allocator to filter out dropped targets #1127

[target-allocator] Add a pre-hook to the allocator to filter out dropped targets #1127

Conversation

moh-osman3 commented Sep 26, 2022 • edited Loading

jaronoff97 left a comment

Choose a reason for hiding this comment

Aneurysm9 left a comment

Choose a reason for hiding this comment

Aneurysm9 Oct 19, 2022

Choose a reason for hiding this comment

moh-osman3 Oct 25, 2022

Choose a reason for hiding this comment

jaronoff97 left a comment

Choose a reason for hiding this comment

kristinapathak left a comment

Choose a reason for hiding this comment

Aneurysm9 Oct 31, 2022

Choose a reason for hiding this comment

moh-osman3 Oct 31, 2022

Choose a reason for hiding this comment

moh-osman3 commented Sep 26, 2022 •

edited

Loading