feat: add new check for binary size in prs - #2731
Conversation
📦 Binary size
No baseline found for |
6257d82 to
8a7e4f5
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
be325fa to
0bb72c0
Compare
| secrets: | ||
| slack_webhook_url: ${{ secrets.slack_webhook_url }} |
There was a problem hiding this comment.
Why not use secrets inherit and avoid changes on the component?
I'm not against it, but curious about the reasoning.
There was a problem hiding this comment.
nr_account_id, nr_license_key, gpg_, and pfx_ are already passed one by one, and neither this workflow or nightly/prerelease use inherit anywhere else. Switching would break that pattern just to save two lines.
It would also give component_send_warning_via_slack.yml access to every secret this workflow currently has the GPG signing key, the PFX cert, the license key even though it only needs the webhook URL. That's a lot of extra secrets exposed for no real reason.
There was a problem hiding this comment.
nr_account_id, nr_license_key, gpg_, and pfx_ are already passed one by one, and neither this workflow or nightly/prerelease use inherit anywhere else. Switching would break that pattern just to save two lines.
The nightly uses
notify-failure:
if: ${{ always() && failure() }}
needs:
- onhost-e2e
- k8s_canaries
- onhost_canaries
- security-image
- security-source-code
- build-image
- build-packages
- k8s-e2e-tests
uses: ./.github/workflows/component_send_warning_via_slack.yml
with:
message: "Nightly workflow failed"
secrets: inherit
It would also give component_send_warning_via_slack.yml access to every secret this workflow currently has the GPG signing key, the PFX cert, the license key even though it only needs the webhook URL. That's a lot of extra secrets exposed for no real reason.
Okey, I see the issue. It's not a security issue though, because we control the other component and what secrets we use. Would it make sense to keep it consistent and inherit all secrets? Then in a different PR, replace all calls to this component and just pass the needed secret? Should we then also change component_image or other workflows to just pass in the required secrets?
There was a problem hiding this comment.
There was a problem hiding this comment.
I think we need a sustain task. Your call
- Use secrets inherit for consistency + create security task
- Leave as it is + create security task
0bb72c0 to
bbe7a8d
Compare
|
|
||
| if [[ -n "$PREVIOUS_SIZE_BYTES" ]]; then | ||
| growth_pct=$(awk -v cur="$SIZE_BYTES" -v prev="$PREVIOUS_SIZE_BYTES" \ | ||
| 'BEGIN { printf "%.2f", (prev == 0) ? 0 : ((cur - prev) * 100.0 / prev) }') |
There was a problem hiding this comment.
Should we print something if the prev size bytes is 0?
It's not the same going from 21 bytes to 21 bytes than going from 0 bytes to 21 bytes. The first case is legitimate, the second seems like an error.
There was a problem hiding this comment.
we now log explicitly when the previous size is 0 and skip the growth check, instead of silently treating 0→N as 0% growth
| # Fetch the previous size before reporting the current one, so we don't race our own insert. | ||
| PREVIOUS_SIZE_BYTES="" | ||
| if [[ -n "${NR_USER_API_KEY:-}" ]]; then | ||
| nrql_query="SELECT sizeBytes FROM AgentControlStats WHERE binaryName = '${BINARY_NAME}' AND target = '${BINARY_TARGET}' SINCE 90 days ago ORDER BY timestamp DESC LIMIT 1" |
There was a problem hiding this comment.
This query is getting the last AgentControlStats, but we are not using triggerEvent nor "$GITHUB_EVENT_NAME" anywhere.
I don't understand how we make sure that we compare a nightly size with the previous nightly size.
There was a problem hiding this comment.
only nightly (schedule/workflow_dispatch) and real pre-releases (release) ever populate AgentControlStats every other caller leaves nr_account_id/nr_license_key unset, so the script exits before reporting. Both build the binary identically, so they're comparable. Added an explicit GITHUB_EVENT_NAME allow-list in the script so this isn't just implicit.
| GORELEASER_CURRENT_TAG: ${{ inputs.tag_name }} | ||
| SKIP_WINDOWS_SIGN: ${{ env.SKIP_WINDOWS_SIGN }} | ||
|
|
||
| - name: Report binary sizes to New Relic |
There was a problem hiding this comment.
This file is called on component_onhost_e2e, which is called on:
- nightly
- prerelease
- push_pr_test_extended_labels
- push_pr_test_extended_paths_host
Does this mean that we will also get a slack message when the check fails on a PR executing onhost e2e tests? I think it does and we don't want that.
There was a problem hiding this comment.
If that's true, one solution I can think of is to just rely on the nightly slack message we already send on error. Then, add something similar to prerelease.
notify-failure:
if: ${{ always() && failure() }}
needs:
- onhost-e2e
- k8s_canaries
- onhost_canaries
- security-image
- security-source-code
- build-image
- build-packages
- k8s-e2e-tests
uses: ./.github/workflows/component_send_warning_via_slack.yml
with:
message: "Nightly workflow failed"
secrets: inherit
There was a problem hiding this comment.
component_onhost_e2e.yml never forwards the NR secrets to this workflow, so the script exits before writing any warning no Slack fire today on PR runs. Also added an explicit github.event_name check on the notify-binary-size-growth job itself, so the protection isn't just implicit secret-wiring.
bbe7a8d to
e1c1da7
Compare
3 participants
Summary
Adds a binary size check for Agent Control: a PR comment that tracks size against
main, plus reporting and alerting for nightly/release builds.newrelic-agent-control/newrelic-agent-control-clion every PR build and posts a sticky comment with size and Δ vs the last successful build onmain(bytes/% delta). Purely informational, doesn't block merge. Falls back gracefully to "no baseline found" when there's nothing to compare against yet (first run after enabling this, or the baseline artifact expired).AgentControlStatscustom event to New Relic onnightly.ymlandprerelease.ymlruns, queryable via NRQL.component_send_warning_via_slack.yml.Changes
.github/workflows/push_pr_checks_tests.ymlbinary size measurement, baseline artifact (published on push tomain, downloaded on PRs), sticky PR comment..github/workflows/scripts/report_binary_size.shnew script: reports size to the Events API, checks growth via NerdGraph, writes a warning file if >10%..github/workflows/component_packages.ymlnew step calling the script per binary; newnotify-binary-size-growthjob that warns on Slack when triggered..github/workflows/component_send_warning_via_slack.ymlaccepts an explicitslack_webhook_urlsecret.