Consider coalescing identical SD definitions

[brian-brazil]

There may be cases where users have multiple different scrape configs that share a SD configuration, but where other settings such as relabelling are different.

We have been slowly adding the ability to filter down at the SD level in an ad-hoc way for each SD, but given the number of things that a Prometheus is likely monitoring I think there's another potential approach to keeping load on the SD sane.

If there's two identical SD configs, let's only instantiate it once and then feed it into each required scrape config. This means a naieve config will have constant load on SD relative to the number of scrape configs, and avoid a good chunk of users having to worry about SD performance.

[fabxc]

👍 that generalisation makes a lot of sense. I guess that's good enough
for k8s as well for single roles.

On Tue, Nov 15, 2016, 4:56 PM Brian Brazil notifications@github.com wrote:

There may be cases where users have multiple different scrape configs that
share a SD configuration, but where other settings such as relabelling are
different.

We have been slowly adding the ability to filter down at the SD level in
an ad-hoc way for each SD, but given the number of things that a Prometheus
is likely monitoring I think there's another potential approach to keeping
load on the SD sane.

If there's two identical SD configs, let's only instantiate it once and
then feed it into each required scrape config. This means a naieve config
will have constant load on SD relative to the number of scrape configs, and
avoid a good chunk of users having to worry about SD performance.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#2191, or mute the thread
https://github.com/notifications/unsubscribe-auth/AEuA8vwsmjwzWW7OfLaux1tTdJb3SGnrks5q-dYegaJpZM4KysP4
.

[brian-brazil]

We've seen already one user concerned about this for EC2, and given Azure's very low rate limits I imagine it'd be really useful there too.

[grandbora]

Hi all,

I want to take a stab at this issue. I dug into the code a bit and formalized a vague idea of how to do this. Below are the details of what I am thinking and couple of questions related to that.

Current flow:

• prometheus starts
  • main instantiates and runs a targetManager, and applies the config
  • targetManager reload loops over each scrapeConfig
    • a targetSet is created for each scrapeConfig
    • UpdateProviders is called for each targetSet with the ProvidersFromConfig

in UpdateProviders two things are done:

• clean up (cancel existing providers, remove all the target groups)
• start listening new providers one by one for each targetSet

Proposed flow
First I plan to break apart the steps of UpdateProviders I mentioned above. There will be two methods;

• CleanUp(Reset): does the clean up step for each targetSet of the targetManager
• ListenChanges: starts listening a targetProvider and apply changes to all the targetSets that rely on it.

With the changes above the flow I would like to implement is:

• prometheus starts
  • main instantiates and runs a targetManager, and applies the config
  • targetManager reload loops over each scrapeConfig
    • a map of targetProvider to TargetSets (map[TargetProvider][]TargetSet) collects the targetSets that use the same Target provider
  • reload calls CleanUp method, resets all targetSets
  • reload loops over targetProvider map, calls ListenChanges once for each provider

Questions:

1. Does the suggestion above make sense? Am I missing anything?
2. Can I rely on reflect.DeepEqual to check if the serviceDiscoveryConfig's are equal?
3. Seems like TargetManager is missing some tests. At the moment I rely on my local setup to manually test my changes, which can simulate fileSD only. Any suggestions on how to test various SD integrations? I tried to setup k8s cluster on a cloud provider, it was a hassle.
4. notifier package accesses to the UpdateProviders method. In case I go with the suggested plan I'll need to make changes in this package as well. Are there any objections, pitfalls I need to be aware of?

cc @fabxc @brian-brazil

[brian-brazil]

Does the suggestion above make sense? Am I missing anything?

I think this logic should live entirely inside the discovery part of the codebase, and not be tied to the other parts of Prometheus or otherwise leak from the discovery api. The reason for this is that we wish at some point to offer sd as a library for use by others, and this is something that should Just Work.

Can I rely on reflect.DeepEqual to check if the serviceDiscoveryConfig's are equal?

Yes.

Any suggestions on how to test various SD integrations?

That's a separate project :)

In case I go with the suggested plan I'll need to make changes in this package as well. Are there any objections, pitfalls I need to be aware of?

The API should remain the same, so no changes should be required.

[grandbora]

Thanks for the comments, I'm planning to submit a pr accordingly.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.