ocurrent / current-bench

Experimental benchmarking infrastructure using OCurrent pipelines
Apache License 2.0
33 stars 17 forks source link

Including a top-level "name" in metrics json causes graph to not be updated #408

Closed gridbugs closed 1 year ago

gridbugs commented 1 year ago

In the readme, under "JSon format", the example has a top-level name field, but if I include a name it causes graphs to not be updated.

If my benchmarks prints out this, the graph on autumn.ocamllabs.io is updated:

{
  "results": [
    {
      "name": "name-of-the-test",
      "metrics": [
        {
            "name": "time",
            "value": 15,
            "units": "sec",
            "description": "x"
        }
      ]
    }
  ]
}

...but if I add a top-level name it is no longer updated:

{
  "name": "foo",
  "results": [
    {
      "name": "name-of-the-test",
      "metrics": [
        {
            "name": "time",
            "value": 15,
            "units": "sec",
            "description": "x"
        }
      ]
    }
  ]
}
art-w commented 1 year ago

The optional name creates separate tabs on the left to separate different benchmarks: (it's "Default" if unspecified)

cb-names

(unless I misunderstood and we have a bug?)

gridbugs commented 1 year ago

🤦 ah yes you're totally right, I didn't see the benchmarks tabs on the left. Most of the time any given revision will only have a single top-level benchmark name, right? If there are multiple top-level names in a repo's history, would there be value in making it clear which one is associated with the current revision, and showing the graphs for that dataset by default?

art-w commented 1 year ago

If you use multiple top-level names, then it's expected that each revision should produce all of them in the make bench output (if not, then it's likely that something went wrong during the execution!)

(As a result, we are not very good at discovering that a metric has been deprecated vs missing, it will stay visible for a very long time but shaded to indicate a lack of recent values... but yeah we don't do this for tabs currently)

gridbugs commented 1 year ago

ah ok that makes sense. Thanks for the info!