Jenkins job for validation summary clickhouse table - #54
sammacbeth wants to merge 20 commits into
Conversation
| currentBuild.description = "Import: ${env.DATE_TO_USE}" | ||
| } | ||
| dir('pixel-schema') { | ||
| checkout([$class: 'GitSCM', branches: [[name: "sam/dashboard"]], |
There was a problem hiding this comment.
Jenkins job checks out developer feature branch
High Severity
The pixel-schema repository checkout uses branch sam/dashboard which appears to be a personal feature branch rather than an official branch. All other repos in this Jenkinsfile correctly use main or develop. This means the production Jenkins job would be pulling from an unstable development branch instead of the main codebase.
Please tell me if this was useful or not with a 👍 or 👎.
There was a problem hiding this comment.
Will fix this just before merging so as not to break the current job.
| ) | ||
| ENGINE = ReplicatedReplacingMergeTree() | ||
| PARTITION BY (date) | ||
| ORDER BY (agent, prefix, params) |
There was a problem hiding this comment.
ClickHouse table ORDER BY causes data deduplication
Medium Severity
The ReplicatedReplacingMergeTree table has ORDER BY (agent, prefix, params) but stores distinct pixel, pixel_id, and version values in separate columns. Since these columns aren't in the ORDER BY, rows with different pixels that share the same agent, prefix, and params will be deduplicated during ClickHouse merges, causing silent data loss including loss of freq aggregations.
Please tell me if this was useful or not with a 👍 or 👎.
| userRemoteConfigs: [[url: 'https://github.com/duckduckgo/duckduckgo-privacy-extension.git']]]) | ||
| } | ||
| dir('pixel-schema') { | ||
| sh "/bin/bash scripts/detailedValidation.sh ../duckduckgo-privacy-extension/pixel-definitions/ ${env.DATE_TO_USE}" |
There was a problem hiding this comment.
Command injection via unvalidated DATE parameter
High Severity
The DATE parameter is used directly in shell commands without validation. A malicious value like 2024-01-01; malicious_command would be interpreted by the shell, executing arbitrary commands on the Jenkins agent. The parameter description suggests YYYY-MM-DD format but doesn't enforce it, allowing command injection via ${env.DATE_TO_USE} in sh steps.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (2)
cab33c6 to
cb05cab
Compare
| } | ||
| const [prefix, pixelMatch] = matchPixel(pixel, this.#compiledPixels); | ||
| this.#currentPixelState.prefix = prefix; | ||
| this.#currentPixelState.pixelMatch = pixelMatch || ''; |
cb05cab to
43aa502
Compare
|
|
||
| SELECT date, agent, version, pixel_id, pixel, params_fixed AS params, COUNT(*) AS freq | ||
| FROM metrics.pixels | ||
| WHERE date = '${day}' AND |
There was a problem hiding this comment.
SQL injection risk in date parameter interpolation
Medium Severity
The day parameter from user input is directly interpolated into the SQL query string at WHERE date = '${day}' without sanitization or parameterization. While the Jenkins job is internal and the Clickhouse client appears read-only (ddg-ro-ch), a user with Jenkins access could provide a malicious DATE parameter to manipulate the query (e.g., 2024-01-01' OR '1'='1).
Additional Locations (1)
| @@ -0,0 +1,19 @@ | |||
| CREATE DATABASE IF NOT EXISTS pixels ON CLUSTER 'ch-prod-cluster'; | |||
|
|
|||
| CREATE TABLE IF NOT EXISTS pixels.validation_results_2 ON CLUSTER 'ch-prod-cluster' ( | |||
There was a problem hiding this comment.
Any reason for the _2 in pixels.validation_results_2?
| @@ -0,0 +1,19 @@ | |||
| CREATE DATABASE IF NOT EXISTS pixels ON CLUSTER 'ch-prod-cluster'; | |||
There was a problem hiding this comment.
File name should probably be validation_table_results.sql
| owners Array(String), | ||
| errors Array(String) | ||
| ) | ||
| ENGINE = ReplicatedReplacingMergeTree() |
There was a problem hiding this comment.
ReplicatedReplacingMergeTree() generally needs a merge key to "replace" effectively, otherwise it relies on insert order, so I think you may have a duplicate data issue here. For example:
SELECT countDistinct(*) FROM pixels.validation_results_2
1095553439
vs.
SELECT countDistinct(*) FROM pixels.validation_results_2 FINAL
899781731
Also the cursorbot comment below re "ORDER BY" is worth digging into IMO.
| pixel_id String, | ||
| pixel String, | ||
| prefix String, | ||
| params Array(String), |
There was a problem hiding this comment.
Array(String) vs String can have a large performance penalty, see https://app.asana.com/1/137249556945/task/1211328177475138
| SUM(CASE WHEN status = 1 THEN freq ELSE 0 END) AS old_app_version, | ||
| SUM(CASE WHEN status = 0 THEN freq ELSE 0 END) AS undocumented, | ||
| COUNT(DISTINCT params) AS parameter_permutations | ||
| FROM pixels.validation_results |
There was a problem hiding this comment.
Table name mismatch between insert and aggregation queries
High Severity
The detailedValidation.sh script inserts data into pixels.validation_results_2, but the aggregation queries in validation_table_aggregations.mjs read FROM pixels.validation_results (without the _2 suffix). The aggregation step won't find any of the just-inserted data, so pixels.daily_validation_results and pixels.daily_valid_prefix_results will get empty or stale rows.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (2)
| GROUP BY date, agent, prefix`; | ||
|
|
||
| spawnSync('ddg-rw-ch', ['-h', 'clickhouse', '--query', query]); | ||
| spawnSync('ddg-rw-ch', ['-h', 'clickhouse', '--query', query2]); |
There was a problem hiding this comment.
Silent failure when ClickHouse aggregation queries error
Medium Severity
The spawnSync return values for both ClickHouse queries are never checked. If either query fails (e.g., connection error, syntax error, missing table), the script exits successfully with no indication of failure. The error output and non-zero exit status from spawnSync are silently discarded.
Please tell me if this was useful or not with a 👍 or 👎.
| GROUP BY date, agent, prefix`; | ||
|
|
||
| spawnSync('ddg-rw-ch', ['-h', 'clickhouse', '--query', query]); | ||
| spawnSync('ddg-rw-ch', ['-h', 'clickhouse', '--query', query2]); |
There was a problem hiding this comment.
spawnSync errors silently ignored in aggregation script
Medium Severity
The return values of both spawnSync calls are discarded. spawnSync does not throw on non-zero exit codes — it returns an object with a status field. If either ClickHouse query fails, the script exits successfully with no error indication, leaving the aggregation tables silently empty or stale.
9723797 to
8d530ed
Compare
| SELECT DISTINCT pixel_id FROM metrics.pixels_validation_pixel_ids | ||
| ) AND | ||
| NOT is_test AND | ||
| version_major >= ${major} |
There was a problem hiding this comment.
Computed version_minor unused in query WHERE clause
Medium Severity
The SQL CTE computes version_minor and the JavaScript destructures minor and patch from the resolved version, but the WHERE clause only filters on version_major >= ${major}. This means versions with the correct major but a lower minor version (e.g., 7.1.0 when minimum is 7.150.0) will be incorrectly included in the results.
Please tell me if this was useful or not with a 👍 or 👎.
| @@ -0,0 +1,139 @@ | |||
| pipeline { | |||
| agent { label 'ubuntu-latest' } | |||
| triggers { cron('H 1 * * 0') } | |||
There was a problem hiding this comment.
Weekly cron only processes a single day's data
Medium Severity
The cron trigger H 1 * * 0 runs weekly (Sundays), but the default date is only "yesterday" (one day). This means six out of seven days are never processed automatically. For a dashboard measuring daily validation status, this likely needs to be a daily schedule (H 1 * * *) or the job needs to iterate over the past week's dates.


Adds a Jenkins job that imports pixel data from Clickhouse, runs validation to generate rows for a validation summary table. This can be used for dashboards to measure pixel traffic and validation status, as well as identify issues with specific pixels.
Note
Medium Risk
Adds new scheduled CI automation that reads from and writes to ClickHouse and introduces new aggregation tables, so misconfiguration or query issues could impact dashboards/data volume. Core validation logic changes are small, but the new pipeline/scripts broaden operational surface area.
Overview
Adds a new scheduled
Jenkinsfile.dashboardpipeline that (optionally per-platform) checks out pixel definition repos, runs a newscripts/detailedValidation.shworkflow for a given date (defaulting to yesterday), and marks individual platform stages as UNSTABLE on failure.Introduces a ClickHouse-backed detailed validation ingestion path:
fetch_pixels_for_day.mjsexports daily pixel traffic to CSV,validate_live_pixel.mjscan emit per-row JSONL results (and optionally skip legacy result file writes viaDISABLE_RESULT_SAVING), anddetailedValidation.shinserts those rows into a newpixels.validation_results_2table defined invalidation_table.sql.Adds
validation_table_aggregations.mjsto compute daily agent-level and prefix-level rollups intopixels.daily_validation_resultsandpixels.daily_valid_prefix_results, and enhances Asana per-owner reports to link pixel names directly to the Grafana pixel dashboard (with agent inferred from the repo path). Also updatesLivePixelsValidator.validatePixelto persistprefix/match info on the returned validation state for downstream consumers.Written by Cursor Bugbot for commit 9b5056b. This will update automatically on new commits. Configure here.