-
Notifications
You must be signed in to change notification settings - Fork 4
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
prometheus: add scrapeInterval #595
Conversation
WalkthroughThis update introduces a new field, Changes
Sequence Diagram(s)sequenceDiagram
participant AR as ApplicationReconciler
participant PG as Prometheus Generator
participant K8s as Kubernetes API
AR->>PG: Call Generate(reconciliation)
PG->>K8s: Create/Update Prometheus resource (ServiceMonitor/PodMonitor)
K8s-->>PG: Return resource status
PG-->>AR: Generation complete
sequenceDiagram
participant SR as SKIPJobReconciler
participant PG as Prometheus Generator
participant K8s as Kubernetes API
SR->>PG: Invoke generateForSkipJob(reconciliation)
PG->>K8s: Create/Update monitoring resource for SKIPJob
K8s-->>PG: Acknowledge update
PG-->>SR: Return completion status
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
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: 1
🧹 Nitpick comments (2)
pkg/resourcegenerator/podmonitor/pod_monitor.go (1)
4-6
: Remove unused import.The
strings
package is imported but not used in this file.-import ( - "fmt" - "strings" - - skiperatorv1alpha1 "github.com/kartverket/skiperator/api/v1alpha1" +import ( + "fmt" + skiperatorv1alpha1 "github.com/kartverket/skiperator/api/v1alpha1"tests/application/service-monitor/application-simple-assert.yaml (1)
104-105
: Refined formatting for NetworkPolicy ports.
The update to the ports section in the NetworkPolicy—ensuring the- port: istio-metrics
entry is clearly defined—enhances readability and maintains consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/v1alpha1/application_types.go
(2 hunks)pkg/resourcegenerator/podmonitor/pod_monitor.go
(3 hunks)pkg/resourcegenerator/servicemonitor/service_monitor.go
(3 hunks)tests/application/service-monitor/application-istio-assert.yaml
(1 hunks)tests/application/service-monitor/application-simple-assert.yaml
(2 hunks)tests/application/service-monitor/patch-application-allowall-assert.yaml
(1 hunks)tests/skipjob/podmonitor/patch-skipjob-remove-monitoring-error.yaml
(1 hunks)tests/skipjob/podmonitor/patch-skipjob-with-monitoring-assert.yaml
(1 hunks)tests/skipjob/podmonitor/skipjob-with-monitoring-assert.yaml
(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CRD and ClusterRole
api/v1alpha1/application_types.go
[failure] 341-341:
extra arguments provided: "s" (at :1:3)
[failure] 341-341:
extra arguments provided: "s" (at :1:3)
🪛 GitHub Check: Build and run tests
api/v1alpha1/application_types.go
[failure] 341-341:
extra arguments provided: "s" (at :1:3)
🪛 GitHub Actions: Test
api/v1alpha1/application_types.go
[error] 341-341: extra arguments provided: 's' (at :1:3)
🪛 GitHub Actions: Build and Deploy Skiperator
api/v1alpha1/application_types.go
[error] 341-341: Extra arguments provided: 's'.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
pkg/resourcegenerator/podmonitor/pod_monitor.go (2)
29-32
: LGTM! Good error handling for scrape interval.The error handling for parsing the scrape interval is well implemented.
51-51
: LGTM! Proper interval configuration.The scrape interval is correctly configured in the PodMonitor spec.
pkg/resourcegenerator/servicemonitor/service_monitor.go (2)
38-41
: LGTM! Good error handling for scrape interval.The error handling for parsing the scrape interval is well implemented.
54-54
: LGTM! Proper interval configuration.The scrape interval is correctly configured in the ServiceMonitor spec.
tests/skipjob/podmonitor/patch-skipjob-with-monitoring-assert.yaml (1)
14-14
: LGTM! Test configuration updated correctly.The test configuration correctly includes the new scrape interval setting.
tests/application/service-monitor/patch-application-allowall-assert.yaml (1)
12-12
: Added scrape interval in ServiceMonitor endpoints.
The newinterval: "30s"
field is correctly added under theendpoints
section, explicitly defining the Prometheus scrape frequency. This change is consistent with the newScrapeInterval
configuration and supports the PR objectives.tests/skipjob/podmonitor/patch-skipjob-remove-monitoring-error.yaml (1)
14-14
: Added scrape interval in PodMonitor configuration.
The addition ofinterval: "30s"
underpodMetricsEndpoints
ensures that metrics for this PodMonitor are scraped at the desired frequency. The change is straightforward and aligns with similar updates in other resources.tests/skipjob/podmonitor/skipjob-with-monitoring-assert.yaml (1)
58-58
: Explicitly defined scrape interval in PodMonitor.
The inclusion ofinterval: "30s"
within the PodMonitor’s specification standardizes the metric collection frequency across configurations. This is a clear and well-integrated implementation in line with the overall monitoring enhancements.tests/application/service-monitor/application-simple-assert.yaml (1)
60-60
: Scrape interval added in ServiceMonitor endpoints.
The newinterval: "30s"
setting in the ServiceMonitor's endpoints section correctly establishes the desired metric scraping interval. It harmonizes with the overall configuration changes introduced in the PR.tests/application/service-monitor/application-istio-assert.yaml (1)
71-71
: Scrape interval specified for ServiceMonitor endpoints.
The addition ofinterval: "30s"
in the ServiceMonitor configuration effectively sets the metrics scraping interval and is consistent with the changes across the test files. This update meets the PR objective and is well implemented.
f627e9a
to
ee00d15
Compare
4094117
to
8b0c12b
Compare
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.
This is beyond my knowledge-set
This offloads the validation to the API server
@coderabbitai review |
✅ Actions performedReview triggered.
|
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
🔭 Outside diff range comments (1)
pkg/resourcegenerator/prometheus/pod_monitor.go (1)
64-64
: Correct the log message to reflect the generated resourceThe log message incorrectly states "Finished generating configmap" instead of "Finished generating podmonitor". This could cause confusion when debugging or reviewing logs.
Apply this diff to correct the log message:
- ctxLog.Debug("Finished generating configmap", "name", skipJob.GetName()) + ctxLog.Debug("Finished generating podmonitor", "name", skipJob.GetName())
🧹 Nitpick comments (4)
tests/application/service-monitor/application-simple-custom-interval.yaml (1)
1-19
: Approved: Application and Namespace configuration with custom scrapeInterval.
The configuration clearly defines a Namespace and an Application resource with a custom scrapeInterval set to90s
. For consistency, consider quoting the interval (e.g.,"90s"
) as seen in other files.tests/skipjob/podmonitor/patch-skipjob-with-monitoring-custom-interval-assert.yaml (1)
1-18
: Approved: PodMonitor configuration for SKIPJob.
The PodMonitor resource is properly set up with the correct endpoint details and uses an interval of"100s"
—consistent with the SKIPJob configuration. As a minor note, you might review the quoting style across files for uniformity.tests/application/service-monitor/application-simple-custom-interval-assert.yaml (1)
1-24
: Approved: ServiceMonitor resource with custom scrapeInterval and metric relabelings.
This ServiceMonitor is configured to match the application’s custom interval ("90s"
) and includes a well-defined metricRelabelings section. For consistency purposes, review the usage of quotes for interval values across configurations.config/crd/skiperator.kartverket.no_applications.yaml (1)
950-967
: New Field: ScrapeInterval in Prometheus Section
The addition of thescrapeInterval
field in the Prometheus section follows the same pattern as in the SKIPJob CRD, ensuring consistency across resource definitions. The configuration (default value, type, and validations) is clearly specified. The validations, which utilize regex matching and numerical boundary checks, ensure that only values meeting the criteria are accepted.Consider adding an inline comment or an example (e.g., "e.g., '30s' or '2m'") near the field description to guide CRD users regarding accepted formats.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
api/v1alpha1/application_types.go
(2 hunks)cmd/skiperator/main.go
(1 hunks)config/crd/skiperator.kartverket.no_applications.yaml
(1 hunks)config/crd/skiperator.kartverket.no_skipjobs.yaml
(1 hunks)internal/controllers/application.go
(2 hunks)internal/controllers/skipjob.go
(2 hunks)pkg/resourcegenerator/prometheus/pod_monitor.go
(2 hunks)pkg/resourcegenerator/prometheus/prometheus.go
(1 hunks)pkg/resourcegenerator/prometheus/service_monitor.go
(2 hunks)tests/application/service-monitor/application-istio-assert.yaml
(1 hunks)tests/application/service-monitor/application-simple-assert.yaml
(2 hunks)tests/application/service-monitor/application-simple-custom-interval-assert.yaml
(1 hunks)tests/application/service-monitor/application-simple-custom-interval-invalid.yaml
(1 hunks)tests/application/service-monitor/application-simple-custom-interval.yaml
(1 hunks)tests/application/service-monitor/chainsaw-test.yaml
(1 hunks)tests/application/service-monitor/patch-application-allowall-assert.yaml
(1 hunks)tests/skipjob/podmonitor/chainsaw-test.yaml
(1 hunks)tests/skipjob/podmonitor/patch-skipjob-remove-monitoring-error.yaml
(1 hunks)tests/skipjob/podmonitor/patch-skipjob-with-monitoring-assert.yaml
(1 hunks)tests/skipjob/podmonitor/patch-skipjob-with-monitoring-custom-interval-assert.yaml
(1 hunks)tests/skipjob/podmonitor/patch-skipjob-with-monitoring-custom-interval.yaml
(1 hunks)tests/skipjob/podmonitor/skipjob-with-monitoring-assert.yaml
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/application/service-monitor/application-simple-custom-interval-invalid.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/skipjob/podmonitor/patch-skipjob-with-monitoring-assert.yaml
- tests/application/service-monitor/application-istio-assert.yaml
- tests/application/service-monitor/patch-application-allowall-assert.yaml
- tests/application/service-monitor/application-simple-assert.yaml
- tests/skipjob/podmonitor/skipjob-with-monitoring-assert.yaml
- tests/skipjob/podmonitor/patch-skipjob-remove-monitoring-error.yaml
🔇 Additional comments (14)
tests/skipjob/podmonitor/chainsaw-test.yaml (1)
21-25
: Custom Scrape Interval Test Step AddedThe new test step applies the patch file
patch-skipjob-with-monitoring-custom-interval.yaml
and asserts the changes usingpatch-skipjob-with-monitoring-custom-interval-assert.yaml
. This explicitly tests the newscrapeInterval
configuration and aligns with the PR objectives to introduce flexible monitoring intervals. The YAML structure and indentation are correct, and the new test step integrates well with the existing test cases.pkg/resourcegenerator/prometheus/service_monitor.go (3)
1-1
: Package name change is appropriateRenaming the package to
prometheus
accurately reflects its broader responsibility in handling Prometheus resources, enhancing code organization and readability.
14-16
: Generator registration is correctly implementedThe
init
function properly registersgenerateForApplication
with themultiGenerator
, ensuring that the resource generation for applications is included during reconciliation.
52-52
: Scrape interval correctly set usinggetScrapeInterval
The scrape interval for Prometheus is appropriately set using the
getScrapeInterval
function, allowing for customizable intervals based on the application's Prometheus configuration.pkg/resourcegenerator/prometheus/prometheus.go (2)
19-25
: VerifyScrapeInterval
validation to ensure only sane values are acceptedCurrently,
getScrapeInterval
returnspc.ScrapeInterval
directly without validating the input. To prevent users from setting unconventional scrape intervals like"23s"
, please verify that theScrapeInterval
is validated to accept only standard and reasonable intervals such as"10s"
,"15s"
,"30s"
, or"1m"
.
1-26
: Prometheus resource generation implementation is well-structuredThe new
prometheus
package provides a clear and modular approach to generating Prometheus resources. TheGenerate
andgetScrapeInterval
functions are well-implemented, improving maintainability.cmd/skiperator/main.go (1)
58-58
: LGTM! Good logging practice.Explicitly setting
DestWriter
toos.Stdout
makes the logging behavior clear and predictable.internal/controllers/skipjob.go (1)
17-17
: LGTM! Consistent resource generation update.The change from
podmonitor
toprometheus
package aligns with the PR's objective and maintains consistency with similar changes inapplication.go
.Also applies to: 166-166
internal/controllers/application.go (1)
27-27
: LGTM! Consistent resource generation update.The change from
servicemonitor
toprometheus
package aligns with the PR's objective and maintains consistency with similar changes inskipjob.go
.Also applies to: 210-210
api/v1alpha1/application_types.go (2)
6-6
: LGTM! Required import for timestamps.The
time
import is correctly added for timestamp handling inFillDefaultsStatus
.
338-346
: LGTM! Well-structured scrape interval configuration.The
ScrapeInterval
field is well-designed with:
- Clear validation rules for minimum intervals (15s or 1m)
- Values in seconds must be divisible by 5
- Sensible default of "60s"
- Proper pattern validation that addresses previous pipeline failures
tests/skipjob/podmonitor/patch-skipjob-with-monitoring-custom-interval.yaml (1)
1-18
: Approved: New SKIPJob resource with monitoring settings.
The file introduces a SKIPJob resource that includes a custom scrapeInterval of"100s"
. Please verify that the value"100s"
conforms to the CRD validation rules (e.g., meeting the minimum interval criteria).tests/application/service-monitor/chainsaw-test.yaml (1)
26-32
: Approved: Enhanced testing steps for custom interval configurations.
The newly added try block correctly applies the custom interval configuration, asserts its expected state, and handles an error case via the invalid configuration file. Please verify that the referenced files (application-simple-custom-interval.yaml
,application-simple-custom-interval-assert.yaml
, andapplication-simple-custom-interval-invalid.yaml
) exist in the expected locations.config/crd/skiperator.kartverket.no_skipjobs.yaml (1)
883-900
: New Field: ScrapeInterval Validation and Configuration
The newly addedscrapeInterval
field is well integrated into the Prometheus section. The default value of60s
and the detailed validation rules—checking that the value is either empty/default or follows the<number>s
or<number>m
format with minimum values (15s for seconds and 1m for minutes) and divisibility constraints—provide robust configuration against invalid inputs. The error messages are clear and informative.
When using CEL expressions the validation rules are now checked in the API server before eventually hitting skiperator for valid manifests |
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.
LGTM
Summary by CodeRabbit
New Features
Refactor
Tests