Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upload native build statistics #39784

Merged
merged 1 commit into from
May 28, 2024

Conversation

zakkak
Copy link
Contributor

@zakkak zakkak commented Mar 29, 2024

Since Quarkus CI already runs the native tests quite often with a fixed
Mandrel version, we can gather these data and increase our insight
regarding Quarkus changes that affects image build time statistics.

@quarkus-bot quarkus-bot bot added the area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure label Mar 29, 2024
Comment on lines 36 to 40
# update timing info if present
if [ -e "$ts" ]; then
curl -s -w '\n' -H "Content-Type: application/json" -H "token: $TOKEN" \
-X PUT --data "@$ts" "$URL/$stat_id" > /dev/null
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be needed as the build time is part of the latest build-output.jsons. It would be better to feed the quarkus version as an update with the new /update-runner-info endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I am waiting for the new RunnerInfo to get deployed to collector.foci.life first (see Karm/collector#23 (comment))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Either way the PUT call for timing info is redundant these days and should not be included.

Copy link
Member

@Karm Karm Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zakkak If you enable GHA on your Q fork, you can switch the endpoint to stage and run it of your own G account, right?

Copy link
Contributor Author

@zakkak zakkak Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will do this, and also enable the job to run on forks, it currently is supposed to run only on the main repo on the main branch.

@zakkak zakkak force-pushed the 2024-03-29-upload-native-build-stats branch from dddcfe0 to 70ddd16 Compare April 22, 2024 13:17
@gsmet
Copy link
Member

gsmet commented Apr 22, 2024

Note that you could use an approach similar to:

- name: Output JSON file
run: |
if [ -f "${{ steps.setup.outputs.build-metadata-file-path }}" ]; then
echo "```json" >> $GITHUB_STEP_SUMMARY
jq '.' ${{ steps.setup.outputs.build-metadata-file-path }} >> $GITHUB_STEP_SUMMARY
echo "\n```" >> $GITHUB_STEP_SUMMARY;
fi

to also add it as a step summary (which allows to visualize things right away instead of downloading the file)

@zakkak zakkak force-pushed the 2024-03-29-upload-native-build-stats branch 3 times, most recently from 8fd0d3b to 9368eb7 Compare April 23, 2024 12:16
@zakkak
Copy link
Contributor Author

zakkak commented Apr 23, 2024

to also add it as a step summary (which allows to visualize things right away instead of downloading the file)

That would be a json file per integration-test, wouldn't it be too much noise in the report? What do you think would make more sense to be shown in the report?

@zakkak zakkak force-pushed the 2024-03-29-upload-native-build-stats branch 2 times, most recently from 6c87de6 to ec781e2 Compare April 24, 2024 05:53
@quarkus-bot

This comment has been minimized.

@zakkak zakkak force-pushed the 2024-03-29-upload-native-build-stats branch 3 times, most recently from 724d42d to 3c4133b Compare April 24, 2024 06:11
@quarkus-bot

This comment has been minimized.

@zakkak zakkak force-pushed the 2024-03-29-upload-native-build-stats branch 4 times, most recently from ad6c821 to 42d9ba9 Compare April 24, 2024 07:00
@quarkus-bot

This comment has been minimized.

@zakkak
Copy link
Contributor Author

zakkak commented Apr 24, 2024

This is blocked by Karm/collector#27

@quarkus-bot

This comment has been minimized.

@zakkak zakkak force-pushed the 2024-03-29-upload-native-build-stats branch 3 times, most recently from ae8263e to d41c47c Compare May 13, 2024 13:21
@zakkak zakkak force-pushed the 2024-03-29-upload-native-build-stats branch from d41c47c to c23eb7f Compare May 27, 2024 07:52
@quarkus-bot

This comment has been minimized.

@zakkak zakkak force-pushed the 2024-03-29-upload-native-build-stats branch 2 times, most recently from 959576e to 5fa39fb Compare May 27, 2024 07:53
@zakkak zakkak force-pushed the 2024-03-29-upload-native-build-stats branch 3 times, most recently from 661ee75 to 61ae566 Compare May 27, 2024 11:50
@quarkus-bot

This comment has been minimized.

@zakkak zakkak force-pushed the 2024-03-29-upload-native-build-stats branch from 61ae566 to 874d587 Compare May 27, 2024 12:25
@quarkus-bot

This comment has been minimized.

@zakkak zakkak force-pushed the 2024-03-29-upload-native-build-stats branch from 874d587 to 61380b8 Compare May 27, 2024 13:00
@zakkak zakkak marked this pull request as ready for review May 27, 2024 13:00
@zakkak
Copy link
Contributor Author

zakkak commented May 27, 2024

This seems to work (tested with Main IT in https://github.com/zakkak/quarkus/actions/runs/9254781855)

@Karm please have a look.

{
"test_version": "$GITHUB_REF_NAME",
"quarkus_version": "$GITHUB_SHA",
"jdk_version": "$(java -version 2>&1 | head -n 2 | tail -n 1)",
"operating_system": "$RUNNER_OS",
"architecture": "$RUNNER_ARCH",
"memory_size_bytes": "$(awk '/MemTotal/ {print $2 * 1024}' /proc/meminfo)",
"memory_available_bytes": "$(awk '/MemAvailable/ {print $2 * 1024}' /proc/meminfo)",
"description": "Quarkus CI github runner on $GITHUB_REF_NAME branch/tag",
"triggered_by": "Quarkus CI"
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Runner info extracted here might be different from the actual GHA runner that ran the native integration test, no? Should we not collect the info where the native tests ran?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing I expect to actually change is memory_available_bytes as AFAIK all free-tier github runners have the same resources. So I am not sure if it's worth creating a new runner for each IT-category (since each category runs on its own runner). No strong opinions though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe we should keep the runner info empty (or some value indicating the values are somewhat meaningless), so as to avoid concluding something from the runner info. That's the only worry. The only info relevant for quarkus should be the test_version and quarkus_version, right?

Copy link
Contributor Author

@zakkak zakkak May 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe we should keep the runner info empty (or some value indicating the values are somewhat meaningless), so as to avoid concluding something from the runner info.

That makes sense

The only info relevant for quarkus should be the test_version and quarkus_version, right?

Right, that should be enough for #40076

Done.

@quarkus-bot

This comment has been minimized.

@zakkak zakkak force-pushed the 2024-03-29-upload-native-build-stats branch from 61380b8 to 2512ca5 Compare May 27, 2024 15:27
@zakkak zakkak requested a review from jerboaa May 27, 2024 15:27
Copy link
Contributor

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me.

@quarkus-bot

This comment has been minimized.

Since Quarkus CI already runs the native tests quite often with a fixed
Mandrel version, we can gather these data and increase our insight
regarding Quarkus changes that affects image build time statistics.
@zakkak zakkak force-pushed the 2024-03-29-upload-native-build-stats branch from 2512ca5 to d470e57 Compare May 28, 2024 07:26
@quarkus-bot
Copy link

quarkus-bot bot commented May 28, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit d470e57.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging-kafka/deployment

io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

  • Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

📦 extensions/smallrye-reactive-messaging/deployment

io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector - History

  • Expecting actual: ["-4","-5","-6","-7","-8","-9","-10","-11"] to start with: ["-3", "-4", "-5", "-6"] - java.lang.AssertionError
java.lang.AssertionError: 

Expecting actual:
  ["-4","-5","-6","-7","-8","-9","-10","-11"]
to start with:
  ["-3", "-4", "-5", "-6"]

	at io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector(ConnectorChangeTest.java:36)

📦 integration-tests/reactive-messaging-kafka

io.quarkus.it.kafka.KafkaConnectorTest.testFruits - History

  • Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.untilAsserted(ConditionFactory.java:790)
	at io.quarkus.it.kafka.KafkaConnectorTest.testFruits(KafkaConnectorTest.java:63)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)

Copy link
Member

@Karm Karm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems sane to me and consistent with this use case: https://github.com/Karm/mandrel-integration-tests/blob/bfb82325b3d3a4838a0e5567d77097ad03a22b36/aux_scripts/run_qts_host.sh

@zakkak The only nitpick would be some perhaps needlessly missing attributes from runner info, e.g.

  "operating_system": "$(uname -o)",
  "architecture": "$(uname -m)",
  "memory_size_bytes": $(awk '/MemTotal/ {print $2 * 1024}' /proc/meminfo),
  "memory_available_bytes": $(awk '/MemAvailable/ {print $2 * 1024}' /proc/meminfo),

@zakkak
Copy link
Contributor Author

zakkak commented May 28, 2024

@zakkak The only nitpick would be some perhaps needlessly missing attributes from runner info, e.g.

Yes, we had that in place but as suggested by @jerboaa in #39784 (comment) I agree it is better we remove them given that they don't come from the actual runner (the runner uploading the data is different than the runner that did the actual runners).

If you think we really need this info I can modify it (in a follow up PR) to use a runner-info per IT-category and report the actual numbers.

@zakkak zakkak merged commit af19dfc into quarkusio:main May 28, 2024
52 checks passed
@zakkak zakkak deleted the 2024-03-29-upload-native-build-stats branch May 28, 2024 13:29
@quarkus-bot quarkus-bot bot added this to the 3.12 - main milestone May 28, 2024
zakkak added a commit to zakkak/quarkus that referenced this pull request May 29, 2024
Follow up to quarkusio#39784

Resolves:

```
jq: error: Could not open file /home/runner/work/quarkus/quarkus/./integration-tests/simple: No such file or directory
```

by treating each line returned by `find` as a single path instead of
further breaking it down by space.
holly-cummins pushed a commit to holly-cummins/quarkus that referenced this pull request Jul 31, 2024
Follow up to quarkusio#39784

Resolves:

```
jq: error: Could not open file /home/runner/work/quarkus/quarkus/./integration-tests/simple: No such file or directory
```

by treating each line returned by `find` as a single path instead of
further breaking it down by space.
danielsoro pushed a commit to danielsoro/quarkus that referenced this pull request Sep 20, 2024
Follow up to quarkusio#39784

Resolves:

```
jq: error: Could not open file /home/runner/work/quarkus/quarkus/./integration-tests/simple: No such file or directory
```

by treating each line returned by `find` as a single path instead of
further breaking it down by space.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure triage/flaky-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants