Add documentation for JsonImporter#146
Conversation
|
Hi @henrikingo, I've added documentation for JsonImporter based on the current implementation. Could you please review and let me know if any changes are needed? Thanks! |
|
Hi @henrikingo, It looks like the GitHub Actions workflow didn’t run due to fork security restrictions. Could you please approve the workflow so the checks can run? Thanks! |
|
Thank you Can you please execute the commands in the new documentation and copy paste the output in this discussion. If there are files involved thrn please cat or head their contents. |
|
@henrikingo — I've verified the commands from the JSON documentation and confirmed they work as expected. Below is the minimal example I used for validation. JSON File (
|
|
Yes, now with the added files it makes more sense :-) Can you move the new files so that they are under I will add some other comments too, but now we have some meaningful direction to start from. |
docs/JSON.md
Outdated
|
|
||
| ## Overview | ||
|
|
||
| `JsonImporter` reads benchmark results from a local JSON file and feeds them into Otava for change-point analysis. It is the simplest data source to set up — no external database or service is required. |
There was a problem hiding this comment.
Json and Csv both have this property.
docs/JSON.md
Outdated
| ## Example Usage | ||
|
|
||
| Run analysis on a test backed by a JSON file: | ||
| otava analyze my_benchmark |
There was a problem hiding this comment.
Please change this to the command that you are using in your reply today:
otava analyze my_benchmark --config otava.yaml
otava.yaml
Outdated
| @@ -0,0 +1,5 @@ | |||
| tests: | |||
There was a problem hiding this comment.
...and this under /examples/json/config/
|
Hi @henrikingo, I’ve addressed all the requested changes: Please let me know if anything else needs improvement. Thanks! |
docs/JSON.md
Outdated
|
|
||
| - **Type:** integer (Unix epoch seconds) | ||
| - **Required:** yes | ||
| - Identifies when the benchmark run occurred. Used for time-range filtering via `DataSelector`. |
There was a problem hiding this comment.
Sorry I missed this the first time. Actually, this should be the time that the commit was merged to the branch we are tracking. Typically this is recorded in git's history as a merge-commit.
In most cases this should NOT be the time that the benchmark was run. In particular, it often happens that you want to rerun a benchmark with the system under test ghecked out and built from this same githash. In such a case, the new results should be recorded with this same timestamp. For the same githash, the timestamp should be constant, regardless of when or how many times the benchmark was run.
henrikingo
left a comment
There was a problem hiding this comment.
Ok thanks! The structure is clearer now and I think this will work.
I have several more comments now on wording or style issues. This is normal when you contribute to a project the first time, even for experienced developers.
I tried to be thorough. Hopefully after this, there won't be many or any more changes.
docs/JSON.md
Outdated
| - Each object must have: | ||
| - `name` (string) — unique identifier for the metric within this run | ||
| - `value` (number) — the measured value | ||
| - Metric names are collected dynamically across all entries in the file. Names must be consistent across runs for change-point analysis to be meaningful. |
There was a problem hiding this comment.
I suggest removing the first sentence. It sound like it is describing the intrenals of how our code works. But this is a user manual, they don't need to understand that. The second sentence can stay though.
docs/JSON.md
Outdated
| ### `attributes` | ||
|
|
||
| - **Type:** object (string → string) | ||
| - **Required:** yes if `branch` filtering is used |
There was a problem hiding this comment.
=> So really you are saying Required: no
docs/JSON.md
Outdated
|
|
||
| ## Example Usage | ||
|
|
||
| Run analysis on a test backed by a JSON file: |
There was a problem hiding this comment.
Suggest to change:
"Analyze test results stored in JSON format."
| tests: | ||
| my_benchmark: | ||
| type: json | ||
| file: test_data/sample.json |
There was a problem hiding this comment.
This needs updating. The user should be able to try the provided example and it should work.
| "attributes": { | ||
| "branch": "main" | ||
| } | ||
| } |
There was a problem hiding this comment.
You'd better add more than one data point if you want to demo change point detection :-)
There was a problem hiding this comment.
Two points also is not enough to trigger a change point...
Perhaps you should use the same data that is in the csv example (local_sample.csv). Just convert that to json.
…ibutes, fix paths, and update examples
|
HI @henrikingo ,Thanks for the detailed feedback! I’ve addressed all the points: Please let me know if anything else needs adjustment. |
|
Hi @henrikingo, I’ve updated the JSON example to include multiple realistic data points based on the CSV example, so it now supports meaningful change-point detection. Let me know if anything else should be improved. |
|
Hi @henrikingo, I realized that some unrelated Sphinx documentation changes were included in this PR when merging branches. If you’d prefer, I can separate those into a different PR and keep this one focused only on the JsonImporter documentation changes. Please let me know how you’d like me to proceed |
|
Sounds like you want to learn about the rebase command so you can undo that last merge com. Once you manage to do that, use |
This PR adds documentation for the JsonImporter.
The documentation explains:
Closes #128
Let me know if any changes are needed.