-
Notifications
You must be signed in to change notification settings - Fork 177
chore: Cloudflare modernization #1472
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
base: master
Are you sure you want to change the base?
chore: Cloudflare modernization #1472
Conversation
Dasomeone
left a comment
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.
Couple comments, mostly all nits or minor things!
Overall looks good, but it'd be great if we could verify it with a test environment? Even a few screenshots would be a good sanity check :)
| "uid": "${datasource}" | ||
| }, | ||
| "expr": "increase(cloudflare_zone_requests_cached{job=~\"$job\", instance=~\"$instance\", zone=~\"$zone\"}[$__interval:]) / increase(cloudflare_zone_requests_total{job=~\"$job\", instance=~\"$instance\", zone=~\"$zone\"}[$__interval:])", | ||
| "expr": "increase(cloudflare_zone_requests_cached{job=~\"integrations/cloudflare\",job=~\"$job\",cluster=~\"$cluster\",zone=~\"$zone\",script_name=~\"$script_name\",instance=~\"$instance\"}[$__interval:]) / increase(cloudflare_zone_requests_total{job=~\"integrations/cloudflare\",job=~\"$job\",cluster=~\"$cluster\",zone=~\"$zone\",script_name=~\"$script_name\",instance=~\"$instance\"}[$__interval:])", |
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 query is missing an increase offset :)
cloudflare-mixin/config.libsonnet
Outdated
| metricsSource: 'prometheus', | ||
|
|
||
| // CloudflareMetricsDown alert filter variable | ||
| alertsMetricsDownJobName: 'integrations/cloudflare', |
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.
Minor nit, we should probably extract this and reuse it in the filteringSelector so that both are updated dynamically in case end users change it
cloudflare-mixin/alerts.libsonnet
Outdated
| { | ||
| alert: 'CloudflareHighRequestRate', | ||
| expr: ||| | ||
| sum without (instance) (100 * (rate(cloudflare_zone_requests_total[10m]) / clamp_min(rate(cloudflare_zone_requests_total[50m] offset 10m), 1))) > %(alertsHighRequestRate)s |
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.
Just noticed this is comparing 10 mins versus the previous 50m. I know that cleanly adds up to an hour, but it feels like an odd way to represent it.
| ), | ||
|
|
||
| 'cloudflare-geomap-overview.json': | ||
| g.dashboard.new(prefix + ' Geomap overview') |
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.
Minor nit, sentence case
| g.dashboard.new(prefix + ' Geomap overview') | |
| g.dashboard.new(prefix + ' geomap overview') |
cloudflare-mixin/panels.libsonnet
Outdated
| + g.panel.timeSeries.fieldConfig.defaults.custom.withSpanNulls(false), | ||
|
|
||
| geoMetricByCountryGeomapPanel: | ||
| g.panel.geomap.new('Geographic Distribution') |
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.
| g.panel.geomap.new('Geographic Distribution') | |
| g.panel.geomap.new('Geographic distribution') |
cloudflare-mixin/panels.libsonnet
Outdated
| description='Duration of worker execution.' | ||
| ) | ||
| + g.panel.timeSeries.standardOptions.withUnit('s') | ||
| + g.panel.timeSeries.fieldConfig.defaults.custom.withFillOpacity(54) |
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.
+1 here
cloudflare-mixin/panels.libsonnet
Outdated
| description='Rate of requests to the worker.' | ||
| ) | ||
| + g.panel.timeSeries.standardOptions.withUnit('reqps') | ||
| + g.panel.timeSeries.fieldConfig.defaults.custom.withFillOpacity(0) |
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.
Should be 10 or greater
cloudflare-mixin/panels.libsonnet
Outdated
| description='Number of errors from the worker.' | ||
| ) | ||
| + g.panel.timeSeries.standardOptions.withUnit('short') | ||
| + g.panel.timeSeries.fieldConfig.defaults.custom.withFillOpacity(0) |
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.
+1
| }, | ||
| signals: { | ||
| geoMapByCountry: { | ||
| name: '$geo_metric Distribution', |
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.
Maybe I'm still mentally on vacation, but I don't recall seeing this used in the name before, just wanted to call out that it's a cool usage!
| + link.link.options.withKeepTime(true), | ||
| otherDashboards: | ||
| link.dashboards.new('All dashboards', this.config.dashboardTags) | ||
| + link.dashboards.options.withIncludeVars(true) |
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.
We'll want to add withIncludeVars(true) to each of the manual entries above as well
| local optional_labels = { | ||
| script_name+: { | ||
| allValue: '.*', | ||
| label: 'Script', | ||
| }, | ||
| }; |
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.
| local optional_labels = { | |
| script_name+: { | |
| allValue: '.*', | |
| label: 'Script', | |
| }, | |
| }; | |
| local optional_labels = { | |
| cluster+: { | |
| allValue: '.*', | |
| }, | |
| script_name+: { | |
| allValue: '.*', | |
| label: 'Script', | |
| }, | |
| zone+: { | |
| allValue: '.*', | |
| label: 'Zone', | |
| }, | |
| }; |
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.
Worker data coming in was missing the zone label, causing no data to show up in the worker dashboard vs. the old dashboards.
…asing, worker data
- Modernize geoMetricByCountryGeomapPanel with full geomap configuration - Added transformations (groupBy, organize) - Configured geomap layers, basemap, controls, and view - Set color mode to continuous-BlPu - Increased panel height from 12 to 24 for better visibility - Modernize geoMetricsByCountryTablePanel - Use shared geoTransformations for DRY code - Added field overrides for gauge visualization on Total/Mean/Last - Configured sort by Total column descending - Modernize poolStatusPanel - Added joinByField and organize transformations - Configured Health column with color-coded mappings (green=Healthy, red=Unhealthy) - Set proper field ordering and renaming - Configuration improvements - Added shared geoTransformations helper to eliminate duplication - Set interval to 2m with intervalFactor 2 for geographic panels - Use standard withTargets approach instead of asTable() for consistency
…n-additions Dashboard and signal updates





This pull request is similar to #1454 where we use more modular libs but now we are using them for Cloudflare.
As of submission this PR is mostly validated via git diff.