-
Notifications
You must be signed in to change notification settings - Fork 94
feat(collector): prometheus input #2425
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
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new Prometheus metrics input in the collector package. A new file is added that implements metrics collection using PromQL with a comprehensive setup including configuration, scheduling, query execution, and result handling. Additionally, the pull request updates the HTTP configuration in another file by replacing environment variable references with fixed default values for timeout and retry parameters. Changes
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
collector/benthos/input/run_ai.go (1)
110-113
: Revisit static HTTP configuration.Replacing environment variable references with hardcoded values restricts the ability to customize or tune these parameters in diverse deployment environments. Consider supporting both a default value and an optional environment-based override to preserve flexibility.
collector/benthos/input/prometheus.go (2)
17-45
: Use more specific prefix for field constants if multiple input plugins coexist.While these constants are self-explanatory, adding a distinguishing prefix can reduce collisions in larger projects with many field definitions. For instance, consider using
promFieldURL
orpromFieldSchedule
.
174-305
: Evaluate memory usage in repeated scraping scenarios.Each scrape result is fully retained in
in.store
until read. Under high query frequency or large result sets, memory usage could spike. If real-time processing or streaming is possible, consider streaming partial results or purging them more aggressively.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
collector/benthos/input/prometheus.go
(1 hunks)collector/benthos/input/run_ai.go
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: CI
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
collector/benthos/input/prometheus.go (4)
1-16
: Imports and package initialization look good.The chosen imports (
client_golang
androbfig/cron
) are appropriate for Prometheus API interactions and scheduling. The overall structure sets a clear foundation for the new input component.
57-64
: Registration process is correct.Registering the new
prometheus
input is consistent with how Benthos plugin registration typically occurs. The error panic ensures immediate visibility for misconfiguration.
66-89
: Data structures look appropriate and self-descriptive.
PromQuery
andQueryResult
clearly separate logical concerns of naming and storing intermediate PromQL vector data, aiding future maintenance and readability.
91-172
: Validate scheduling interval logic for edge cases.The code assumes at least two consecutive cron occurrences to compute an interval. For custom or irregular schedules (e.g., one-time or complex cron patterns), this might yield unexpected intervals. Consider adding guards or documentation clarifications about unsupported cron expressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
collector/benthos/input/run_ai.go (1)
110-113
: Evaluate preserving environment variable references.Replacing environment variables with hardcoded values can reduce configuration flexibility for deployments that rely on environment-based overrides. Consider reverting to an environment-based approach or adding an override mechanism to retain dynamic control over these parameters.
Here's an example approach:
http: - timeout: 30s - retry_count: 1 - retry_wait_time: 100ms - retry_max_wait_time: 1s + timeout: "${RUNAI_HTTP_TIMEOUT:30s}" + retry_count: "${RUNAI_HTTP_RETRY_COUNT:1}" + retry_wait_time: "${RUNAI_HTTP_RETRY_WAIT_TIME:100ms}" + retry_max_wait_time: "${RUNAI_HTTP_RETRY_MAX_WAIT_TIME:1s}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
collector/benthos/input/prometheus.go
(1 hunks)collector/benthos/input/run_ai.go
(1 hunks)
🔇 Additional comments (3)
collector/benthos/input/prometheus.go (3)
23-55
: Configuration specification is well-defined.Your
prometheusInputConfig()
function uses a clear structure with object fields for queries and an instructive example. This approach helps users understand how to configure multiple PromQL queries. Great job.
91-172
: Efficient initialization and schedule-based interval setup.In
newPrometheusInput()
, using two subsequent cron occurrences to calculate the interval is a clever technique. This ensures that the input aligns precisely with changing schedules and avoids large or unexpected intervals if the cron expression is updated.
174-305
: Robust concurrency and retry mechanism.The sequence of
scrape()
,Connect()
,ReadBatch()
, andClose()
uses a mutex to ensure thread safety, and any failed batch is re-added to the store for reprocessing. This design effectively addresses concurrency concerns and fault tolerance, with good logging for errors and warnings.
in.mu.Lock() | ||
in.store[t] = results | ||
in.mu.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in.mu.Lock() | |
in.store[t] = results | |
in.mu.Unlock() | |
in.mu.Lock() | |
defer in.mu.Unlock() | |
in.store[t] = results |
Summary by CodeRabbit
New Features
Chores