Introduce RunnerInfo to ImageStats

[zakkak]

This new optional entry is meant to be used for additional information
related to the runner used to perform the testing that generated the
gathered image statistics.

Closes #22

[jerboaa]

This only changes the model. We need to update controllers as well so that we can actually pass in the values on insert/update using HTTP API:

https://github.com/Karm/collector/blob/main/src/main/java/com/redhat/quarkus/mandrel/collector/report/endpoints/GraalImageStatsResource.java#L33
https://github.com/Karm/collector/blob/main/src/main/java/com/redhat/quarkus/mandrel/collector/report/endpoints/ImageStatsResource.java#L175

[zakkak]

Uh, right.

If I get this right:

• GraalImageStatsResource is used to adapt the graal generated json file, so I don't see how/why we should change this as the test_version field won't be available in that file.
• ImageStatsResource doesn't seem to need anything explicitly to work with the new field, expect for getExperiment maybe that already doesn't include the generator_version. Otherwise, list, listOne and getByTag should work as expected.

But in order to be able to actually put a value to that field we will need something similar to ImageStatsResource.updateBuildTime. I am actually thinking about renaming ImageStatsResource.updateBuildTime and adding test_version in GraalBuildInfo instead of creating a new endpoint and a new schema. WDYT?

[jerboaa]

* `GraalImageStatsResource` is used to adapt the graal generated json file, so I don't see how/why we should change this as the `test_version` field won't be available in that file.

Yes, though we inject a tag already there. It's conceivable to pass another parameter which is the test_version value when importing. Note this is the main entrypoint these days. See:
https://github.com/graalvm/mandrel/blob/82e6ba445d7c6dea8e4cdbdf2c8bf0ce8b527753/.github/import_stats.sh#L35

* `ImageStatsResource` doesn't seem to need anything explicitly to work with the new field, expect for `getExperiment` maybe that already doesn't include the generator_version. Otherwise, `list`, `listOne` and `getByTag` should work as expected.

That's right. It relies on the passed in ImageStat to already have the test_version set, though.

But in order to be able to actually put a value to that field we will need something similar to ImageStatsResource.updateBuildTime. I am actually thinking about renaming ImageStatsResource.updateBuildTime and adding test_version in GraalBuildInfo instead of creating a new endpoint and a new schema. WDYT?

It's used here, fwiw (but should probably go away now):
https://github.com/graalvm/mandrel/blob/82e6ba445d7c6dea8e4cdbdf2c8bf0ce8b527753/.github/import_stats.sh#L38-L39

I don't think we need updateBuildTime any more for GraalVM 23+. The GraalStats will have them and the adapter will set the build time correctly. See:

collector/src/main/java/com/redhat/quarkus/mandrel/collector/report/adapter/StatsAdapter.java

Lines 77 to 86 in be4d439

	// Schema 0.9.1 added total time to the graal stats
	final double totalTimeSec = graalStat.getResourceUsage().getTotalTimeSecs();
	if (totalTimeSec > 0) {
	perfStats.setTotalTimeSeconds(totalTimeSec);
	} else {
	// Schema 0.9.0 (22.3):
	// Timing information needs to be updated using the PUT endpoint
	// of api/v1/image-stats/<id>
	perfStats.setTotalTimeSeconds(-1);
	}

So instead of changing the updateBuildTime endpoint, I'd suggest to add a new one, updateAux or something, that - right now - only adds the test_version. Come to think about it more, maybe we should introduce a new object for those "external" infos. Say AuxInfo part of ImageStats. Similar to ImageSizeStats or the like. That would be more future proof should we need more info. But that's up to you.

TLDR;
The following steps should be sufficient:

• Add a new update endpoint for setting the test_version
• Update the script used by GHA's to use that new endpoint

[zakkak]

So instead of changing the updateBuildTime endpoint, I'd suggest to add a new one, updateAux or something, that - right now - only adds the test_version. Come to think about it more, maybe we should introduce a new object for those "external" infos. Say AuxInfo part of ImageStats. Similar to ImageSizeStats or the like. That would be more future proof should we need more info. But that's up to you.

I liked the idea so I went for it. Please have another look.

[Karm]

@zakkak Thx for adding to The Collector!

Ad "Aux" metadata in general, while at it, could we create an entity (add to the one you created) that would take also stuff like this:

https://github.com/Karm/collector/blob/main/src/main/java/com/redhat/quarkus/mandrel/collector/report/model/SimpleTimeAndSize.java#L36-L47

I'd then refactor SimpleTimeAndSize to use it too. I have no hard requirements on the attribute names, I can easily rename tags I look for in jsonc lib. I arrived at the place where I am sure we need at least those runner descriptions, separate version fields so as I can sort by those etc.

I wouldn't shun from adding a String attribute with GH PR link to Aux entity either. On the simple CI this could be used for Q PRs, in the Hydra experiments I'd add Graal PRs etc.

Regarding the architecture and OS for ImageStats, I am abusing the ImageName for it now, producing things like:

build-perf-amd64-el8-q-all
build-perf-aarch64-el8-q-all
build-perf-aarch64-el9-q-all

...which I would like to stop doing :-D

[Karm]

@zakkak Reading my previous comment, I am not sure I am making sense. TL;DR:

1. THX.
2. Pls consider adding more attributes, I will re-use your entity in runtime tests.
3. Pls fix testing, add a test that doesn't use the new endpoint and one that uses it, whitelist sequence warning.

:-D

[zakkak]

@Karm It looks like it makes more sense to add a new class RunnerInfo and possibly another named TestInfo (or BenchmarkInfo) instead of just pushing everything in an Aux.

WDYT?

[Karm]

@Karm It looks like it makes more sense to add a new class RunnerInfo and possibly another named TestInfo (or BenchmarkInfo) instead of just pushing everything in an Aux.

WDYT?

@zakkak
Would it be more by the book: Yes.

Do we need it: IMHO no.

I'd jam it into a simple aux class, sort of metadata. We are not about to develop a database of well defined runners to which we would associate experiments and runs and a set of test apps. IMHO a test app is as simple as a String like "Karm/fuzz@ca881049" or "https://github.com/quarkusio/quarkus/tree/3.9.0/integration-tests/jpa-derby".
For a runner, it's good to know the RAM, cores, arch, os and our description.

Do you feel we need to impose more structure on the topic?

[zakkak]

I think I addressed all your comments PTAL. Note that I decided to go with RunnerInfo instead of AuxiliaryInfo. If you prefer AuxiliaryInfo I can easily do the renaming, just let me know.

[jerboaa]

LGTM. Have you tried the angular UI with this change? Does it break?

[Karm]

Thx @zakkak, lemme take a look 👁️

[zakkak]

LGTM. Have you tried the angular UI with this change? Does it break?

It happily just ignores the new data

[jerboaa]

It happily just ignores the new data

Thanks. Good to know.

[Karm]

@zakkak GHA will deploy it to https://stage-collector.foci.life/ momentarily...

[zakkak]

@Karm when should we expect it to make it to collector.foci.life, as that's were it would make more sense to have for uploading build stats from Quarkus CI.

Do you suggest first setting up Quarkus to upload stats to stage and once we see everything works as expected then move it to prod?

[Karm]

@zakkak I would like to see a JSON returned from stage-collector that contains the data from the full Quarkus run, uplaoded the way you want it, displayed (retrieved) the way you wanted.

Meaning, to see that indeed GHA run 9823793 produces these 23 build.json files that were all uploaded with this one RunnerInfo and thus can be reported as such.

If it's already there, let's see it here...

[Karm]

Releasing:
Unless we are touching schema, it's juts tagging a tag here and running the GHA Workflow that deploys to prod with that tag.

This PR is touching schema, our prod conf is set to "validate", we need "update". The way I do that is physically sshing to the system, dumping DB, changing the prop via env temporarily and staring the service and seeing the log. (The whole system is snapshotted nightly, so even without the dump, worst case, we lose 1 day).

This PR is benign in a way that is adds, not removes stuff/relations. So it should be all good and no manual migration steps ought to be required.

[zakkak]

@Karm stage seems OK with the changes (including your enhancements).

curl -v -H "Content-Type: application/json" -H "token: ${TOKEN}" https://stage-collector.foci.life/api/v1/image-stats/199003

Is an example. Do you think we can push it to production or would you like to get some more testing done first?