Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Grafana integration: Dependabot entries, a new GitHub Actions "Grafana" workflow, a Docker Compose stack (CrateDB, Grafana, example task), a Python provisioning/validation script with requirements, and a test runner script for end‑to‑end verification. Changes
Sequence DiagramsequenceDiagram
participant GHA as GitHub Actions
participant Docker as Docker Compose
participant CDB as CrateDB
participant GF as Grafana
participant Script as example-weather.py
GHA->>Docker: docker compose up --wait -d
Docker->>CDB: start CrateDB container
Docker->>GF: start Grafana container
CDB-->>Docker: healthcheck OK
GF-->>Docker: healthcheck OK
Docker->>Script: docker compose run example-weather
Script->>CDB: insert sample weather data (SQL)
CDB-->>Script: insert OK
Script->>GF: create datasource via Grafana API
GF-->>Script: datasource created / exists
Script->>GF: upload/update dashboard
GF-->>Script: dashboard created/ack
Script->>GF: validate datasource health
GF-->>Script: health OK
Script->>GF: trigger dashboard queries (Grafana -> CrateDB)
GF->>CDB: execute SQL queries
CDB-->>GF: query results
GF-->>Script: query responses (HTTP 200)
Script-->>Docker: validation complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@application/grafana/example-weather.py`:
- Around line 143-147: The current hard assert on
grafana.datasource.health_inquiry(DATASOURCE_UID) can fail due to Grafana's
health-check SQL being incompatible with CrateDB; keep the two logger.info lines
for health.status and health.message but remove the assert and instead run an
explicit lightweight smoke query via the smartquery API (e.g., call smartquery
with a trivial SELECT/limit against DATASOURCE_UID) to determine pass/fail; if
the smartquery returns a successful result treat the datasource as healthy,
otherwise log the smartquery error and raise/assert failure. Ensure you
reference grafana.datasource.health_inquiry, DATASOURCE_UID, health, logger, and
smartquery when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 985816ff-6a03-44ee-ab7d-29b15d1bde41
📒 Files selected for processing (6)
.github/dependabot.yml.github/workflows/application-grafana.ymlapplication/grafana/compose.ymlapplication/grafana/example-weather.pyapplication/grafana/requirements.txtapplication/grafana/test.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- application/grafana/requirements.txt
- application/grafana/test.sh
- application/grafana/compose.yml
| logger.info("Loading data into CrateDB") | ||
|
|
||
| # Load example data into CrateDB. | ||
| dataset = load_dataset("tutorial/weather-basic") |
There was a problem hiding this comment.
Possibly add this to the variable definition block above?
DATASET_NAME = "tutorial/weather-basic"| dataset = load_dataset("tutorial/weather-basic") | |
| dataset = load_dataset(DATASET_NAME) |
amotl
changed the title
amotl
force-pushed
the
grafana
branch
5 times, most recently
from
c527bfc to
d075101
Compare
|
Is it possible that this test rig can also validate the column name autocompletion feature? |
seut
left a comment
seut
left a comment
There was a problem hiding this comment.
thanks! just some versions to check suggestions
| - "11.6" | ||
| - "12.3.4" | ||
| - "12.4" | ||
| - "nightly" |
There was a problem hiding this comment.
I don't think it makes sense to test a grafana nightly, it may fail for valid reasons which could never land in a release.
| - "nightly" |
There was a problem hiding this comment.
I think this would only be the case with main. Using nightly here is the right choice to receive "going south" signals early. If we see too much flakyness due to unrelated instabilities in Grafana nightly, we can always remove that label again. I don't expect many of such, because we are only testing a very minor surface of Grafana, and I don't expect them to ship any completely dysfunctional releases. Grafana nightly is well tested.
There was a problem hiding this comment.
I'd prefer to instead ensure that we update this once a new major or minor version is released (dependbot?). I'd like to avoid any possible effort caused by a nightly version which may never see the light. Anyhow, we can also test and remove it once it causes issues.
| - "9.5.21" | ||
| - "10.4.19" | ||
| - "11.6" | ||
| - "12.3.4" | ||
| - "12.4" |
There was a problem hiding this comment.
should we rather test minor releases instead of very concrete hotfix release?
| - "9.5.21" | |
| - "10.4.19" | |
| - "11.6" | |
| - "12.3.4" | |
| - "12.4" | |
| - "9.5" | |
| - "10.4" | |
| - "11.6" | |
| - "12.3" | |
| - "12.4" |
There was a problem hiding this comment.
I would love to do it that way, but I wasn't able to find corresponding OCI publications.
$ docker pull grafana/grafana-oss:10.4
Error response from daemon: manifest for grafana/grafana-oss:10.4 not found: manifest unknown: manifest unknown
$ docker pull grafana/grafana-oss:11.6
Error response from daemon: manifest for grafana/grafana-oss:11.6 not found: manifest unknown: manifest unknown
There was a problem hiding this comment.
I see.
For grafana >= 11.6 this works:
% docker pull grafana/grafana:11.6
11.6: Pulling from grafana/grafana
....
So lets use the minor tags for all >= 11.6?
Additionally, we should use grafana/grafana instead of grafana/grafana-oss as the grafana-oss is EOL since 12.4, see https://grafana.com/docs/grafana/latest/setup-grafana/installation/docker/.
| SELECT | ||
| $__timeGroupAlias("timestamp", $__interval), | ||
| "location", | ||
| MEAN("{column_name}") AS "{column_name}" | ||
| FROM "example"."weather_data" | ||
| WHERE $__timeFilter("timestamp") | ||
| GROUP BY "time", "location" | ||
| ORDER BY "time" |
There was a problem hiding this comment.
Please note this querying scheme+template is super important to follow, otherwise you will set the wire and the browser on fire when processing big data.
@hammerhead told me about this the other day, but I don't know if it is general knowledge across the board and is correctly applied by all our users and customers. 1
Footnotes
-
I have a slight suspicion users can overload their clusters easily because nobody tells them about those details. In this spirit, a dedicated CrateDB datasource plugin for Grafana would be very sweet indeed, which would optimally guide the user appropriately, similarly like InfluxDB users, for example, who do not need to be concerned about relevant details at all? ↩
There was a problem hiding this comment.
However, in the following comment, you can see @matriv pinged me about the DATE_BIN() function that had been recommended by @seut.
If that works now, I would be happy to demonstrate the modern version here, even if it requires CrateDB 5.7. We can easily add the SQL template above to the documentation to inform users of older CrateDB versions.
There was a problem hiding this comment.
Would be great to do that, 5.7 is already a bit old.
There was a problem hiding this comment.
Did someone use the new variant successfully already, so we can copy from there?
/cc @hammerhead, @grbade, @zolbatar
There was a problem hiding this comment.
@zolbatar will look into this detail. Thank you! 🙇
| - directory: "/application/grafana" | ||
| package-ecosystem: "docker-compose" | ||
| schedule: | ||
| interval: "daily" |
There was a problem hiding this comment.
Wouldn't weekly be enough? (same for the other jobs)
| interval: "daily" | |
| interval: "weekly" |
There was a problem hiding this comment.
We want to be informed in-time when something goes south, so we can start conversations with upstream authors earlier than later.
| SELECT | ||
| $__timeGroupAlias("timestamp", $__interval), | ||
| "location", | ||
| MEAN("{column_name}") AS "{column_name}" | ||
| FROM "example"."weather_data" | ||
| WHERE $__timeFilter("timestamp") | ||
| GROUP BY "time", "location" | ||
| ORDER BY "time" |
There was a problem hiding this comment.
Would be great to do that, 5.7 is already a bit old.
2 participants
About
Review
While CI is failing on some matrix slots, the patch can still be reviewed and merged, because it is valid, as demonstrated on CI slots of earlier Grafana releases. Thanks!
References
grafana-cratedb-sourcecrate-clients-tools#275/cc @karynzv, @hammerhead, @goat-ssh, @zolbatar, @WalBeh