Skip to content

feat(datalayer): configurable per-source scrape interval - #2214

Open
noalimoy wants to merge 2 commits into
llm-d:mainfrom
noalimoy:feat/1880-per-source-scrape-interval
Open

feat(datalayer): configurable per-source scrape interval#2214
noalimoy wants to merge 2 commits into
llm-d:mainfrom
noalimoy:feat/1880-per-source-scrape-interval

Conversation

@noalimoy

@noalimoy noalimoy commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?
/kind feature

What this PR does / why we need it:

All polling data-layer sources share one Collector goroutine per endpoint and one base tick (--refresh-metrics-interval, default 50ms). That cadence fits latency-sensitive model-server metrics, but wastes HTTP traffic for sources that update more slowly (e.g. DCGM ~1s, /v1/models).

Each polling source plugin now accepts an optional interval parameter (alongside scheme, path, port, etc.). The PollingDispatcher interface exposes Interval() time.Duration; the Runtime converts it to a period in base ticks (must be a positive multiple of the base tick) and the Collector dispatches a source only when due. Omitting interval keeps every-tick behavior.

Scheduling uses a tick-based next-due check in the existing Collector loop. A full timing wheel was not needed for the small number of polling sources per endpoint; we can revisit if that grows.

Key design choices:

  • One Collector goroutine per endpoint — no new goroutines; the existing loop checks which sources are due on each tick
  • Tick-based next-due scheduling instead of a full timing wheel — sufficient for the small number of polling sources per endpoint
  • interval lives in plugin parameters — it is source-specific configuration, same as port or scheme
  • ParseIntervalOption centralises duration parsing so factories stay one-liners
  • NewHTTPDataSource rejects negative intervals at construction time

Example:

plugins:
- type: dcgm-data-source
  name: dcgm-source
  parameters:
    port: 9400
    interval: "1s"   # must be a multiple of --refresh-metrics-interval
- type: dcgm-extractor
  name: dcgm-extractor
dataLayer:
  sources:
  - pluginRef: dcgm-source
    extractors:
    - pluginRef: dcgm-extractor

Which issue(s) this PR fixes:
Fixes #1880

Test plan

  • Collector skips non-due sources across ticks (fast vs slow period)
  • Configure rejects intervals that are not a multiple of the base tick
  • Configure stores interval: 1s as 20 period ticks when base is 50ms
  • Negative interval rejected by NewHTTPDataSource constructor
  • make build and make test pass with zero failures

Release note (write NONE if no user-facing change):

Polling data-layer sources accept an optional `interval` plugin parameter (e.g. `interval: "1s"`). It must be a positive multiple of `--refresh-metrics-interval` (default 50ms); when omitted, the source scrapes every base tick.

@noalimoy
noalimoy requested review from a team and shmuelk as code owners July 28, 2026 12:34
@noalimoy
noalimoy requested review from liu-cong and vMaroon July 28, 2026 12:34
@github-actions github-actions Bot added kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 28, 2026
Polling sources shared one Collector tick (~50ms), so slow-changing
sources (DCGM, /v1/models) were scraped far more often than useful.
Schedule each dataLayer.sources entry by a tick multiple of
--refresh-metrics-interval; omit keeps every-tick behavior.

Signed-off-by: noalimoy <nlimoy@redhat.com>
@noalimoy
noalimoy force-pushed the feat/1880-per-source-scrape-interval branch from 397e674 to 8a0ec35 Compare July 29, 2026 11:12
@github-actions github-actions Bot added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Jul 29, 2026
Signed-off-by: noalimoy <nlimoy@redhat.com>
@noalimoy
noalimoy force-pushed the feat/1880-per-source-scrape-interval branch from 8a0ec35 to 97fcf9e Compare July 29, 2026 11:41

@elevran elevran left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @noalimoy - generally can merge.
Left a few questions/nits for your conisderations, especially the note on base frequency defined higher than an interval.

Comment thread docs/architecture.md

Polling sources share one Collector goroutine per endpoint. The base tick is
`--refresh-metrics-interval` (default 50ms). Each polling source plugin accepts an
`interval` parameter (e.g. `"1s"`) that must be a positive multiple of the base tick;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit:

  • does not have to be exact multiple. we can do the math of round-to-nearest(duration/base) in the data layer runtime. In the doc just say that the interval will be rounded to the nearest multiple of base.
  • I'm fine with keeping it as metrics refresh, but it isn't really tied to metrics anymore. Just something to consider. Rename and deprecation of the flag is not worth it, IMHO.

Q:
how do you propose to treat cases where interval < base tick? Right now user needs to be aware and configure correctly. Two options (at least): (1) pick smallest interval as base period (not necessarily --refresh-metrics-interval) and define all others as multiple (2) use --refresh-metrics-interval as base frequency, everything else runs off of it - log a warning if an interval is below and operate the lower values at the base frequency.
I prefer the first option.

side note: lets define a minimal base interval (50ms?) and log a warning if user sets lower.

plugin.Plugin
Dispatch(ctx context.Context, ep Endpoint) error
AppendExtractor(ext plugin.Plugin) error
Interval() time.Duration

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Q/nit:
another option would have been to add a (derived) interface with an additional Interval() method so that only interested collectors need to implement it and not need to add Interval() time.Duration { return 0 } where it is not needed. The runtime can assert and query the interval of only needed implementation.
WDYT?
Not pushing for it - looking for feedback.

if cfg.UseNodeAddress {
opts = append(opts, http.WithUseNodeAddress())
}
if intervalOpt, err := http.ParseIntervalOption(cfg.Interval); err != nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is there a parse duration in the standard lib that would work?

- `insecureSkipVerify` (bool, optional, default: `true`): Skip TLS certificate verification.
- `caCertPath` (string, optional): PEM CA bundle to verify the target's server cert.
- `clientCertPath` / `clientKeyPath` (string, optional): client certificate for mTLS. Set both together.
- `interval` (string, optional): Scrape period (e.g. `"5s"`). Must be a positive

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: the same help string appears in multiple locations. Would it be possible to have it specified once (e.g., might need a data layer README)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: configurable per-source scraping interval in the data layer

2 participants