From 7d7ce140244fbf78823e11195ff176b2e3b00f19 Mon Sep 17 00:00:00 2001 From: Nelson Osacky Date: Tue, 25 Aug 2026 12:45:05 +0200 Subject: [PATCH 1/3] ci(macrobenchmark): Compare the startup benchmark against its merge base (JAVA-679) The Sauce run reported absolute cold-start numbers for one build, which are close to unreadable on a cloud device with unlocked CPU clocks. Build the sample app from the merge base as well, install both on the device under separate application ids, and alternate cold starts between them so thermal drift lands on both and cancels in the difference. Report the delta to the job summary and to a PR comment the workflow keeps updating. Sauce resigns the app under test but never touches dependent apps, so resigning is disabled: left on, the candidate would carry an injected agent the baseline does not and every delta would include its cost. Reports only. It never fails the job, and stays on a manual trigger. --- .../integration-tests-macrobenchmark.yml | 92 ++++++- .../sentry-uitest-android-macrobenchmark.yml | 16 +- scripts/baseline-app-id.init.gradle | 20 ++ scripts/parse-macrobenchmark-log.py | 227 +++++++++++++++--- .../README.md | 114 ++++++--- .../src/main/AndroidManifest.xml | 14 +- .../macrobenchmark/SentryStartupBenchmark.kt | 89 ++++++- 7 files changed, 493 insertions(+), 79 deletions(-) create mode 100644 scripts/baseline-app-id.init.gradle diff --git a/.github/workflows/integration-tests-macrobenchmark.yml b/.github/workflows/integration-tests-macrobenchmark.yml index 95a9ef4832c..8a9ab5bb63a 100644 --- a/.github/workflows/integration-tests-macrobenchmark.yml +++ b/.github/workflows/integration-tests-macrobenchmark.yml @@ -1,6 +1,11 @@ name: 'Integration Tests - Macrobenchmark' # Runs the sentry-uitest-android-macrobenchmark cold-start benchmark on a Sauce Labs real -# device and recovers timeToInitialDisplay from the device log. +# device and recovers its metrics from the device log. +# +# The sample app is built twice -- once from this ref, once from its merge base with main -- and +# both are installed on the device so the benchmark can alternate between them. Absolute numbers +# from a cloud device with unlocked CPU clocks are close to unreadable; a delta measured against +# a baseline on the same device in the same session is not. # on: workflow_dispatch: @@ -14,6 +19,10 @@ jobs: name: Macrobenchmark runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: write # to post the comparison back onto the PR + # we copy the secret to the env variable in order to access it in the workflow env: SAUCE_USERNAME: ${{ secrets.SAUCE_USERNAME }} @@ -21,6 +30,23 @@ jobs: steps: - name: Git checkout uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # Full history: the baseline is the merge base with main, which shallow clones can't find. + fetch-depth: 0 + + - name: Resolve the merge base + id: base + run: | + git fetch --no-tags origin main + echo "sha=$(git merge-base HEAD origin/main)" >> "$GITHUB_OUTPUT" + + # Comparing against the merge base rather than main's tip keeps commits that landed on main + # in the meantime out of the delta. + - name: Git checkout the merge base + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ steps.base.outputs.sha }} + path: baseline - name: 'Set up Java: 17' uses: actions/setup-java@b6effb05e454b25005698d916606bdc6ffcbf961 # v5 @@ -35,6 +61,20 @@ jobs: if: env.SAUCE_USERNAME != null run: ./gradlew :sentry-samples:sentry-samples-android:assembleRelease :sentry-android-integration-tests:sentry-uitest-android-macrobenchmark:assembleBenchmark + # The init script lives in this checkout, not the baseline one, because the merge base + # predates it. Both builds share a Gradle user home, so this one starts with a warm cache. + - name: Assemble the baseline target app + if: env.SAUCE_USERNAME != null + working-directory: baseline + run: ./gradlew --build-cache :sentry-samples:sentry-samples-android:assembleRelease -I "$GITHUB_WORKSPACE/scripts/baseline-app-id.init.gradle" + + - name: Stage the baseline apk for Sauce + if: env.SAUCE_USERNAME != null + run: | + mkdir -p build/macrobenchmark-baseline + cp baseline/sentry-samples/sentry-samples-android/build/outputs/apk/release/sentry-samples-android-release.apk \ + build/macrobenchmark-baseline/ + - name: Run Macrobenchmark in SauceLab uses: saucelabs/saucectl-run-action@283660aa934c02723c497efa151d582a3acc5801 # pin@v3 if: env.SAUCE_USERNAME != null @@ -54,7 +94,55 @@ jobs: # would report success while silently producing no results. set -o pipefail python3 scripts/parse-macrobenchmark-log.py ./artifacts \ - --json-out ./artifacts/benchmarkData.json | tee -a "$GITHUB_STEP_SUMMARY" + --json-out ./artifacts/benchmarkData.json \ + --base-sha "${{ steps.base.outputs.sha }}" \ + --head-sha "${{ github.sha }}" \ + | tee ./artifacts/summary.md >> "$GITHUB_STEP_SUMMARY" + + # workflow_dispatch has no PR context, so the PR is looked up from the dispatched branch. + # Dispatching on a branch without an open PR is fine -- the job summary still has the table. + - name: Comment the comparison on the PR + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + if: ${{ !cancelled() && env.SAUCE_USERNAME != null }} + with: + script: | + const fs = require('fs'); + const marker = ''; + + let body; + try { + body = fs.readFileSync('./artifacts/summary.md', 'utf8'); + } catch { + core.info('No summary to post; the benchmark did not report results.'); + return; + } + // The recovery step is allowed to fail, and `tee` leaves a truncated file behind when + // it does. Only a report that got as far as writing its marker is worth posting. + if (!body.startsWith(marker)) { + core.info('Summary is incomplete; leaving the PR comment alone.'); + return; + } + + const head = `${context.repo.owner}:${context.ref.replace('refs/heads/', '')}`; + const { data: prs } = await github.rest.pulls.list({ ...context.repo, head, state: 'open' }); + if (prs.length === 0) { + core.info(`No open PR for ${head}; results are in the job summary only.`); + return; + } + const issue_number = prs[0].number; + + // Update this workflow's own comment rather than stacking a new one on every dispatch. + const comments = await github.paginate(github.rest.issues.listComments, { + ...context.repo, + issue_number, + }); + const existing = comments.find((comment) => comment.body?.startsWith(marker)); + + if (existing) { + await github.rest.issues.updateComment({ ...context.repo, comment_id: existing.id, body }); + } else { + await github.rest.issues.createComment({ ...context.repo, issue_number, body }); + } - name: Upload Sauce artifacts uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 diff --git a/.sauce/sentry-uitest-android-macrobenchmark.yml b/.sauce/sentry-uitest-android-macrobenchmark.yml index 4e3d5cfbb3d..906d275bad1 100644 --- a/.sauce/sentry-uitest-android-macrobenchmark.yml +++ b/.sauce/sentry-uitest-android-macrobenchmark.yml @@ -11,18 +11,30 @@ sauce: - macrobenchmark defaults: - timeout: 40m + # 24 cold starts plus one full AOT compile per step, against 12 cold starts before the run + # started measuring two builds. + timeout: 60m espresso: app: ./sentry-samples/sentry-samples-android/build/outputs/apk/release/sentry-samples-android-release.apk testApp: ./sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/build/outputs/apk/benchmark/sentry-uitest-android-macrobenchmark-benchmark.apk + otherApps: + # The merge-base build of the same app, applicationId suffixed `.baseline` so it installs + # alongside the one above. The workflow stages it here; when it is missing, saucectl fails + # rather than silently measuring one build. + - ./build/macrobenchmark-baseline/sentry-samples-android-release.apk suites: - - name: "Macrobenchmark startup (api 35)" + - name: "Macrobenchmark startup A/B (api 35)" # No test orchestrator and no clearPackageData: Macrobenchmark manages its own process # restarts and AOT compilation, and StartupMode.COLD intentionally keeps app data and # permissions (it force-stops rather than `pm clear`). + appSettings: + # Sauce resigns the app under test on real devices, but never touches `otherApps`. Left on, + # the candidate would carry an injected agent the baseline does not and every delta would + # include the cost of that agent. + resigningEnabled: false devices: - id: Google_Pixel_9_Pro_XL_15_real_sjc1 # Google Pixel 9 Pro XL - api 35 (15) - high end diff --git a/scripts/baseline-app-id.init.gradle b/scripts/baseline-app-id.init.gradle new file mode 100644 index 00000000000..745ca07b039 --- /dev/null +++ b/scripts/baseline-app-id.init.gradle @@ -0,0 +1,20 @@ +// Suffixes the sample app's applicationId so a second build of it can be installed alongside the +// one under test. Used by the macrobenchmark workflow to put the merge-base build and the PR +// build on the same device at the same time. +// +// This is injected with `-I` rather than committed to the sample's own build script because the +// baseline is built from a checkout of the merge base, which predates this file. +// +// Groovy, not Kotlin: a Kotlin init script would need AGP on its own `initscript` classpath to +// see the types behind the `android` extension, while Groovy resolves it dynamically. +gradle.beforeProject { project -> + if (project.path != ':sentry-samples:sentry-samples-android') { + return + } + // Registered from `beforeProject`, so it runs ahead of the afterEvaluate AGP registers when the + // build script applies it -- late enough to override whatever the build script set, early + // enough that the DSL is still writable. + project.afterEvaluate { + project.android.defaultConfig.applicationIdSuffix = '.baseline' + } +} diff --git a/scripts/parse-macrobenchmark-log.py b/scripts/parse-macrobenchmark-log.py index 280970dbea8..2ff2b8fb6dd 100755 --- a/scripts/parse-macrobenchmark-log.py +++ b/scripts/parse-macrobenchmark-log.py @@ -1,23 +1,41 @@ #!/usr/bin/env python3 -"""Recover Macrobenchmark results from a Sauce Labs device log. +"""Recover Macrobenchmark results from a Sauce Labs device log and compare the two builds. -Sauce Labs cannot pull arbitrary files off a real device, so -SentryStartupBenchmark echoes its `-benchmarkData.json` into logcat as -numbered chunks. This reassembles those chunks and prints a Markdown summary. +Sauce Labs cannot pull arbitrary files off a real device, so SentryStartupBenchmark echoes its +`-benchmarkData.json` into logcat as numbered chunks. This reassembles those chunks and +prints a Markdown summary. + +When the run alternated between the merge-base build and the build under test, the summary is a +base-vs-PR comparison. When only one build was installed -- a plain local run -- it falls back to +reporting that build on its own. Usage: parse-macrobenchmark-log.py [--json-out benchmarkData.json] + [--base-sha SHA] [--head-sha SHA] """ import argparse import json +import random import re +import statistics import sys from pathlib import Path # Must match SentryStartupBenchmark.LOG_TAG and its "[index/total]" chunk prefix. CHUNK_RE = re.compile(r"SentryBenchmarkData\s*:\s*\[(\d+)/(\d+)\](.*)$") +# Must match SentryStartupBenchmark.Variant. These appear in each benchmark's parameterized name, +# e.g. `startup[03-candidate]`. +BASELINE, CANDIDATE = "baseline", "candidate" + +# Enough resamples that the reported p is stable to about a thousandth, and still well under a +# second for the ~24 measurements a run produces. +RESAMPLES = 10000 + +# Fixed so re-running the parser on the same log always prints the same p. +SEED = 0 + def log_messages(log_file): """Yields the message text of every log entry. @@ -40,7 +58,12 @@ def log_messages(log_file): def collect_chunks(log_file): - """Returns one log's chunk texts keyed by index, plus the expected total.""" + """Returns one log's chunk texts keyed by index, plus the expected total. + + The benchmark may dump its results more than once (see logBenchmarkDataToLogcat). Later + chunks overwrite earlier ones and the file only ever grows, so what survives is the last, + most complete document. + """ chunks, total = {}, None for message in log_messages(log_file): match = CHUNK_RE.search(message) @@ -57,56 +80,192 @@ def reassemble(chunks, total): return "".join(chunks[i] for i in range(1, total + 1)) -def format_summary(data): +def variant_of(benchmark_name): + """Which build a benchmark entry measured, or None if the run wasn't labelled.""" + for variant in (BASELINE, CANDIDATE): + if variant in benchmark_name: + return variant + return None + + +def pool_runs(benchmarks): + """Returns {variant: {metric: [every iteration, across all steps]}}. + + Each step's own median and coefficientOfVariation are computed over a handful of iterations + and say nothing useful, so the per-iteration values are pooled per variant and the statistics + recomputed from those. + """ + pooled = {} + for benchmark in benchmarks: + variant = variant_of(benchmark["name"]) + for metric, result in benchmark["metrics"].items(): + pooled.setdefault(variant, {}).setdefault(metric, []).extend(result["runs"]) + return pooled + + +def summarize(runs): + return { + "min": min(runs), + "median": statistics.median(runs), + "max": max(runs), + "cov": statistics.stdev(runs) / statistics.fmean(runs) if len(runs) > 1 else 0.0, + "n": len(runs), + } + + +def permutation_p(base_runs, candidate_runs): + """Two-sided p-value for "the medians differ", by relabelling the measurements at random. + + Distribution-free, which suits a dozen noisy cold starts per arm better than a t-test: it + asks how often shuffling the same measurements between the two labels produces a median gap + at least as big as the one actually observed. A large p means the split we saw is an + unremarkable way to deal out these numbers -- not evidence of no change, just no evidence of + one. + """ + observed = abs(statistics.median(candidate_runs) - statistics.median(base_runs)) + pool = list(base_runs) + list(candidate_runs) + split = len(base_runs) + rng = random.Random(SEED) + at_least_as_extreme = 0 + for _ in range(RESAMPLES): + rng.shuffle(pool) + gap = abs(statistics.median(pool[split:]) - statistics.median(pool[:split])) + if gap >= observed: + at_least_as_extreme += 1 + # Add-one keeps p away from an unachievable 0. + return (at_least_as_extreme + 1) / (RESAMPLES + 1) + + +def format_header(data, base_sha, head_sha): context = data["context"] build = context["build"] lines = [ - "## Macrobenchmark results", - "", f"**Device:** {build['brand']} {build['model']} " f"(api {build['version']['sdk']}, {context['cpuCoreCount']} cores) · " f"**compilation:** {context['compilationMode']} · " f"**CPU clocks locked:** {context['cpuLocked']}", "", ] + if base_sha or head_sha: + lines[:0] = [ + f"**base:** `{base_sha or 'unknown'}` → **PR:** `{head_sha or 'unknown'}`", + "", + ] + return lines + + +def format_runs_details(label, runs): + return [ + "", + f"
{label} per iteration", + "", + ", ".join(f"{run:.1f}" for run in runs), + "", + "
", + ] + + +def format_comparison(data, pooled, base_sha, head_sha): + lines = ["## Macrobenchmark: PR vs merge base", ""] + lines += format_header(data, base_sha, head_sha) - if not context["cpuLocked"]: + if not data["context"]["cpuLocked"]: lines += [ - "> CPU clocks are unlocked on this device, so run-to-run spread is wide. " - "Treat these numbers as a trend, not a regression gate.", + "> CPU clocks are unlocked on this device, so the absolute numbers run high and wide. " + "The two builds were measured alternately on the same device in the same session, so " + "the delta is the trustworthy part -- not the medians either side of it.", "", ] - table = [ - "| Benchmark | Metric | min | median | max | CoV | iterations |", - "|---|---|--:|--:|--:|--:|--:|", + lines += [ + "| Metric | base median | PR median | Δ | Δ% | p | base min–max (CoV) | PR min–max (CoV) |", + "|---|--:|--:|--:|--:|--:|--:|--:|", ] details = [] - for benchmark in data["benchmarks"]: - name = f"{benchmark['className'].rsplit('.', 1)[-1]}.{benchmark['name']}" - for metric, result in sorted(benchmark["metrics"].items()): - table.append( - f"| `{name}` | {metric} " - f"| {result['minimum']:.1f} | {result['median']:.1f} | {result['maximum']:.1f} " - f"| {result['coefficientOfVariation'] * 100:.1f}% | {len(result['runs'])} |" - ) - runs = ", ".join(f"{run:.1f}" for run in result["runs"]) - details += [ - "", - f"
{metric} per iteration", - "", - runs, - "", - "
", - ] - - return "\n".join(lines + table + details) + for metric in sorted(set(pooled[BASELINE]) & set(pooled[CANDIDATE])): + # Only metrics both builds reported can be compared; the rest are called out below. + base_runs, candidate_runs = pooled[BASELINE][metric], pooled[CANDIDATE][metric] + base, candidate = summarize(base_runs), summarize(candidate_runs) + delta = candidate["median"] - base["median"] + delta_pct = delta / base["median"] * 100 if base["median"] else 0.0 + lines.append( + f"| {metric} | {base['median']:.1f} | {candidate['median']:.1f} " + f"| {delta:+.1f} | {delta_pct:+.1f}% " + f"| {permutation_p(base_runs, candidate_runs):.3f} " + f"| {base['min']:.1f}–{base['max']:.1f} ({base['cov'] * 100:.1f}%) " + f"| {candidate['min']:.1f}–{candidate['max']:.1f} ({candidate['cov'] * 100:.1f}%) |" + ) + details += format_runs_details(f"{metric} — base", base_runs) + details += format_runs_details(f"{metric} — PR", candidate_runs) + + iterations = len(next(iter(pooled[CANDIDATE].values()))) + lines += [ + "", + f"Δ is PR minus base, so negative is faster. {iterations} cold starts per build, " + "alternating between them so thermal drift lands on both. `p` is a permutation test on " + "the difference of medians: small means the gap is hard to explain as a reshuffle of the " + "same measurements. Reported for information only — this job never fails on it.", + ] + + one_sided = sorted(set(pooled[BASELINE]) ^ set(pooled[CANDIDATE])) + if one_sided: + lines += [ + "", + "> Only one build reported " + ", ".join(f"`{m}`" for m in one_sided) + ", so " + "there is nothing to compare it against and it is missing from the table above.", + ] + + if pooled.get(None): + lines += [ + "", + "> Some results carried no variant label and were left out of the table. That means " + "the benchmark ran a test this parser doesn't know about.", + ] + + return "\n".join(lines + details) + + +def format_single(data, pooled, base_sha, head_sha): + """Fallback for a run that measured one build, e.g. a local connectedBenchmarkAndroidTest.""" + variant, metrics = next(iter(pooled.items())) + lines = ["## Macrobenchmark results", ""] + lines += format_header(data, base_sha, head_sha) + lines += [ + f"> Only the **{variant or 'unlabelled'}** build was measured, so there is nothing to " + "compare against. Install both builds to get a base-vs-PR table.", + "", + "| Metric | min | median | max | CoV | iterations |", + "|---|--:|--:|--:|--:|--:|", + ] + details = [] + for metric in sorted(metrics): + runs = metrics[metric] + stats = summarize(runs) + lines.append( + f"| {metric} | {stats['min']:.1f} | {stats['median']:.1f} | {stats['max']:.1f} " + f"| {stats['cov'] * 100:.1f}% | {stats['n']} |" + ) + details += format_runs_details(metric, runs) + return "\n".join(lines + details) + + +def format_summary(data, base_sha=None, head_sha=None): + pooled = pool_runs(data["benchmarks"]) + if not pooled: + sys.exit("Benchmark data contains no results") + # The marker lets the workflow find and update its own PR comment instead of adding another. + marker = "" + if BASELINE in pooled and CANDIDATE in pooled: + return f"{marker}\n{format_comparison(data, pooled, base_sha, head_sha)}" + return f"{marker}\n{format_single(data, pooled, base_sha, head_sha)}" def main(): parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("artifacts_dir", type=Path, help="directory of downloaded Sauce artifacts") parser.add_argument("--json-out", type=Path, help="where to write the recovered benchmarkData.json") + parser.add_argument("--base-sha", help="commit the baseline build came from") + parser.add_argument("--head-sha", help="commit the build under test came from") args = parser.parse_args() log_files = sorted(args.artifacts_dir.rglob("*.log")) @@ -135,7 +294,7 @@ def main(): if args.json_out: args.json_out.write_text(json.dumps(data, indent=2)) - print(format_summary(data)) + print(format_summary(data, args.base_sha, args.head_sha)) if __name__ == "__main__": diff --git a/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/README.md b/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/README.md index 9914825584d..3535494a875 100644 --- a/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/README.md +++ b/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/README.md @@ -1,7 +1,8 @@ # sentry-uitest-android-macrobenchmark Jetpack Macrobenchmark for cold-start of `sentry-samples-android`, used to evaluate SDK-init -performance changes on a real device in a **stable, reproducible** way. Not run in CI. +performance changes on a real device in a **stable, reproducible** way. Runs on Sauce Labs from the +`Integration Tests - Macrobenchmark` workflow, on a manual trigger only. ## What it measures @@ -20,10 +21,37 @@ inspect the relevant slices directly (each iteration's trace is saved under `CompilationMode.Full()` pins ART AOT so dexopt state can't drift between runs. `StartupMode.COLD` does the correct force-stop sequencing (it does **not** `pm clear`, so app data/permissions are -kept). Iterations are capped at 12 because back-to-back cold starts thermally throttle an -unlocked-clock device after ~14 iterations, inflating the tail of longer runs. +kept). -## Running +## Comparing two builds + +Absolute cold-start numbers are only meaningful next to a baseline measured under the same +conditions, so the benchmark measures **two builds of the sample app at once**: + +| Variant | Package | Built from | +|---|---|---| +| `CANDIDATE` | `io.sentry.samples.android` | the ref under test | +| `BASELINE` | `io.sentry.samples.android.baseline` | its merge base with `main` | + +Both are installed on the same device and the run alternates between them, so whatever the device +does over the session — thermal throttling above all — lands on both and cancels out in the +difference. Two things follow from that, and both are deliberate: + +- **The run alternates ABBA, not ABAB.** In ABAB the candidate always follows the baseline, so + drift within a pair is charged to the candidate every single time. Mirroring each round makes + each variant the trailing one equally often. +- **The absolute numbers get worse, and that's fine.** 24 alternating cold starts throttle this + class of device where 12 back-to-back ones only start to. Read the delta, not the medians either + side of it. + +The suffixed applicationId comes from `scripts/baseline-app-id.init.gradle`, applied with `-I` when +building the baseline. It is injected rather than committed to the sample's build script because +the baseline is built from a checkout of the merge base, which predates the file. + +If only the candidate is installed, the baseline steps are **skipped** (not failed) and the report +falls back to single-build numbers. + +## Running locally Connect a device, then: @@ -34,11 +62,46 @@ Connect a device, then: Results print to the console and are written to `build/outputs/connected_android_test_additional_output/.../*-benchmarkData.json`. +That measures the candidate only. To get the comparison locally, build the baseline out of a +worktree at the merge base and install it alongside: + +```bash +git worktree add ../baseline "$(git merge-base HEAD origin/main)" +(cd ../baseline && ./gradlew :sentry-samples:sentry-samples-android:assembleRelease \ + -I "$OLDPWD/scripts/baseline-app-id.init.gradle") +adb install -r ../baseline/sentry-samples/sentry-samples-android/build/outputs/apk/release/sentry-samples-android-release.apk +``` + +Then re-run the benchmark. `scripts/parse-macrobenchmark-log.py` also reads `adb logcat` output, so +the same base-vs-PR table can be produced from a local run. + +### Device hygiene (do this for trustworthy numbers) + +- **Wake and unlock the device first** — the launch check fails with "Unable to confirm activity + launch completion" on a dozing/locked screen + (`adb shell input keyevent KEYCODE_WAKEUP && adb shell wm dismiss-keyguard`). +- **Charge above 25%** — Macrobenchmark refuses to run below that. +- **Lock CPU clocks** if the device is rooted: this is the single biggest cure for thermal drift. +- Otherwise: let the device cool between runs, keep it on AC power, enable airplane mode, and turn + animations off (`adb shell settings put global window_animation_scale 0`, plus + `transition_animation_scale` and `animator_duration_scale`). +- Heed Macrobenchmark's warnings about unlocked clocks / low battery — they mean the numbers are + noisy. + ## Running on Sauce Labs The `Integration Tests - Macrobenchmark` workflow (manual trigger) runs the same benchmark on a -Sauce Labs real device. It reports numbers only — it is not a PR gate, because cloud devices run -with unlocked CPU clocks and the run-to-run spread swamps most SDK-init changes. +Sauce Labs real device: it builds the sample app from the dispatched ref and from its merge base, +ships both, and posts the comparison to the job summary and to a PR comment it keeps updating. +It reports numbers only and never fails on them. + +Two configuration details in `.sauce/sentry-uitest-android-macrobenchmark.yml` are load-bearing: + +- **`otherApps`** carries the baseline APK. Sauce installs dependent apps without instrumenting or + modifying them, and allows up to seven. +- **`appSettings.resigningEnabled: false`.** Sauce resigns the app under test on real devices but + never touches `otherApps`. Left on, the candidate would carry an injected agent the baseline does + not, and every delta would include the cost of that agent. Getting the numbers *back off* the device is the awkward part, so if you are changing this, know what has already been ruled out: @@ -58,28 +121,17 @@ what has already been ruled out: So `SentryStartupBenchmark` echoes its own `benchmarkData.json` into logcat in chunks, which reaches CI inside `device.log`, and `scripts/parse-macrobenchmark-log.py` reassembles it and -writes a `timeToInitialDisplay` table to the job summary. Note this recovers the metrics only — -the perfetto traces are megabytes each and cannot go through logcat, so sub-millisecond work -still needs a local device. - -### Device hygiene (do this for trustworthy numbers) - -- **Wake and unlock the device first** — the launch check fails with "Unable to confirm activity - launch completion" on a dozing/locked screen - (`adb shell input keyevent KEYCODE_WAKEUP && adb shell wm dismiss-keyguard`). -- **Charge above 25%** — Macrobenchmark refuses to run below that. -- **Lock CPU clocks** if the device is rooted: this is the single biggest cure for thermal drift. -- Otherwise: let the device cool between runs, keep it on AC power, enable airplane mode, and turn - animations off (`adb shell settings put global window_animation_scale 0`, plus - `transition_animation_scale` and `animator_duration_scale`). -- Heed Macrobenchmark's warnings about unlocked clocks / low battery — they mean the numbers are - noisy. - -## A/B-ing an SDK change - -Macrobenchmark measures one build per run, so compare separate runs — but **interleave them**: -running all of variant A followed by all of variant B lets thermal drift systematically penalize -whichever variant runs second. Instead, alternate A/B rounds (build variant A, run, build variant -B, run, repeat 2–3 times), keep each round's `*-benchmarkData.json`, and compare the values pooled -per variant. Prefer the `SentryAndroid.init` metric for SDK-init changes — it isolates init cost, so -it moves on changes that `timeToInitialDisplay` would bury in cold-start noise. +writes the comparison table to the job summary. Note this recovers the metrics only — the perfetto +traces are megabytes each and cannot go through logcat, so sub-millisecond work still needs a +local device. + +## Known limitations + +- **A merge base older than the `SentryAndroid.init` trace section** (added in #5901) has no such + section for `TraceSectionMetric` to find, and Macrobenchmark fails the whole run rather than + reporting the other metric. Rebase onto a newer `main` if you hit this. +- Both builds carry the same DSN, so baseline launches also send events to the sample project. + Harmless for the measurement. +- `p` in the report is a permutation test on the difference of medians. It says how hard the + observed gap is to explain as a reshuffle of the same measurements; a large `p` is not evidence + that nothing changed. diff --git a/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/AndroidManifest.xml b/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/AndroidManifest.xml index b2d3ea12352..a436d12f449 100644 --- a/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/AndroidManifest.xml +++ b/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/AndroidManifest.xml @@ -1,2 +1,14 @@ - + + + + + + + + + diff --git a/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/java/io/sentry/uitest/android/macrobenchmark/SentryStartupBenchmark.kt b/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/java/io/sentry/uitest/android/macrobenchmark/SentryStartupBenchmark.kt index 824a2a0628f..30cb37675eb 100644 --- a/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/java/io/sentry/uitest/android/macrobenchmark/SentryStartupBenchmark.kt +++ b/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/java/io/sentry/uitest/android/macrobenchmark/SentryStartupBenchmark.kt @@ -1,5 +1,6 @@ package io.sentry.uitest.android.macrobenchmark +import android.content.pm.PackageManager import android.util.Log import androidx.benchmark.macro.CompilationMode import androidx.benchmark.macro.ExperimentalMetricApi @@ -7,13 +8,34 @@ import androidx.benchmark.macro.StartupMode import androidx.benchmark.macro.StartupTimingMetric import androidx.benchmark.macro.TraceSectionMetric import androidx.benchmark.macro.junit4.MacrobenchmarkRule -import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry import java.io.File +import java.util.Locale import org.junit.AfterClass +import org.junit.Assume.assumeTrue import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith +import org.junit.runners.Parameterized + +/** The two builds of the sample app a run can measure. */ +enum class Variant(val packageName: String) { + /** + * The merge-base build, suffixed by `scripts/baseline-app-id.init.gradle` so both can coexist. + */ + BASELINE("io.sentry.samples.android.baseline"), + + /** The build under test. The only variant present on a plain local run. */ + CANDIDATE("io.sentry.samples.android"), +} + +/** One `measureRepeated` call: which build to launch, and where it sits in the run order. */ +data class Step(private val index: Int, val variant: Variant) { + // Becomes the JUnit parameter name, so results arrive as `startup[01-baseline]`. The index keeps + // the run order visible in the raw data; the label is what parse-macrobenchmark-log.py groups on. + override fun toString() = + String.format(Locale.ROOT, "%02d-%s", index, variant.name.lowercase(Locale.ROOT)) +} /** * Cold-start benchmark for the sentry-samples-android app, used to evaluate SDK-init changes on a @@ -26,34 +48,79 @@ import org.junit.runner.RunWith * - SentryAndroid.init ([TraceSectionMetric]) — the duration of the `SentryAndroid.init` * [android.os.Trace] section the SDK emits, isolating SDK-init cost from the rest of the start. * + * When both builds are installed, the run alternates between them ([steps]) so that whatever the + * device does over the session — thermal throttling above all — lands on both and cancels out in + * the difference. Absolute numbers from an alternating run are therefore worse than from a short + * single-build run; the *delta* is what this is for. When only [Variant.CANDIDATE] is installed, + * the baseline steps are skipped and the run degrades to measuring one build. + * * [CompilationMode.Full] pins ART AOT compilation so dexopt state does not drift between runs. */ @OptIn(ExperimentalMetricApi::class) -@RunWith(AndroidJUnit4::class) -class SentryStartupBenchmark { +@RunWith(Parameterized::class) +class SentryStartupBenchmark(private val step: Step) { @get:Rule val benchmarkRule = MacrobenchmarkRule() @Test - fun startupFullCompilation() = + fun startup() { + // A local run normally has only the candidate installed. Skip rather than fail, so the run + // still produces single-build numbers. + assumeTrue( + "${step.variant.packageName} is not installed", + isInstalled(step.variant.packageName), + ) benchmarkRule.measureRepeated( - packageName = TARGET_PACKAGE, + packageName = step.variant.packageName, metrics = listOf(StartupTimingMetric(), TraceSectionMetric(INIT_TRACE_SECTION)), compilationMode = CompilationMode.Full(), startupMode = StartupMode.COLD, - iterations = 12, + iterations = ITERATIONS_PER_STEP, setupBlock = { pressHome() }, ) { startActivityAndWait() } + } - // Not private: @AfterClass needs a public static method. - companion object { - private const val TARGET_PACKAGE = "io.sentry.samples.android" + private fun isInstalled(packageName: String): Boolean = + try { + // Needs the entries in the module's AndroidManifest to see past package visibility. + InstrumentationRegistry.getInstrumentation() + .context + .packageManager + .getPackageInfo(packageName, 0) + true + } catch (e: PackageManager.NameNotFoundException) { + false + } + // Not private: @Parameters and @AfterClass need public static methods. + companion object { // Matches the android.os.Trace section name in SentryAndroid.init. private const val INIT_TRACE_SECTION = "SentryAndroid.init" + /** + * Cold starts per step. Kept small so the run alternates often, but not 1: every step pays for + * its own `CompilationMode.Full` AOT compile of the target. + */ + private const val ITERATIONS_PER_STEP = 3 + + /** How many times [AB_ROUND] repeats. 2 rounds x 3 iterations = 12 cold starts per variant. */ + private const val ROUNDS = 2 + + /** + * ABBA rather than ABAB: in ABAB the candidate always follows the baseline, so any drift within + * a pair is charged to the candidate every single time. Mirroring the second half of each round + * makes each variant the trailing one equally often. + */ + private val AB_ROUND = + listOf(Variant.BASELINE, Variant.CANDIDATE, Variant.CANDIDATE, Variant.BASELINE) + + @JvmStatic + @Parameterized.Parameters(name = "{0}") + fun steps(): List = + List(ROUNDS) { AB_ROUND }.flatten().mapIndexed { index, variant -> Step(index + 1, variant) } + private const val BENCHMARK_DATA_SUFFIX = "-benchmarkData.json" /** Kept in sync with `scripts/parse-macrobenchmark-log.py`. */ @@ -73,6 +140,10 @@ class SentryStartupBenchmark { * Safe to run at this point because `ResultWriter` writes the file synchronously as each result * is appended; only its *reporting* is deferred to the end of the run. Locally this is just * extra logcat noise — Gradle still copies the real file into the build directory. + * + * [Parameterized] runs this once, after the last step, so one dump covers every step. Were that + * ever to change, the parser would still cope: the file only grows, and its later-chunk-wins + * reassembly ends up with the most complete document in the log. */ @JvmStatic @AfterClass From b051c2584f77f3b3cdd29e11cad76376a9b9ec4f Mon Sep 17 00:00:00 2001 From: Nelson Osacky Date: Tue, 25 Aug 2026 15:03:58 +0200 Subject: [PATCH 2/3] ci(macrobenchmark): Trigger the comparison from pull requests (JAVA-679) Running from a pull request hands the workflow the context it previously had to reconstruct, which removes most of the machinery around the comparison itself. The PR lookup is gone: with a pull_request trigger the comment is a stock sticky-comment action rather than hand-written JS that resolved the PR from the dispatched ref. The merge-base computation is gone too, because the checkout is already refs/pull/N/merge and the base is simply its first parent. That also sharpens the comparison -- the candidate is the PR merged into its base, so anything that landed on the base since the PR forked is present in both arms and cancels out. The applicationId suffix moves from a Gradle init script into a sampleAppIdSuffix property on the sample app. The init script only existed because the baseline is built from a checkout that predated it, which stops being true once this is on main. Until then the base cannot suffix its build, so both APKs would share a package and the benchmark would quietly compare an app against itself; the staging step now compares the two application ids and fails loudly instead. Gated on a run-macrobenchmark label. A run costs two Gradle builds and up to an hour of a Sauce device at concurrency 1, which is far too much for every push. --- .../integration-tests-macrobenchmark.yml | 121 ++++++++---------- .../sentry-uitest-android-macrobenchmark.yml | 2 +- scripts/baseline-app-id.init.gradle | 20 --- scripts/parse-macrobenchmark-log.py | 10 +- .../README.md | 45 ++++--- .../macrobenchmark/SentryStartupBenchmark.kt | 4 +- .../sentry-samples-android/build.gradle.kts | 3 + 7 files changed, 91 insertions(+), 114 deletions(-) delete mode 100644 scripts/baseline-app-id.init.gradle diff --git a/.github/workflows/integration-tests-macrobenchmark.yml b/.github/workflows/integration-tests-macrobenchmark.yml index 8a9ab5bb63a..09c91aece9d 100644 --- a/.github/workflows/integration-tests-macrobenchmark.yml +++ b/.github/workflows/integration-tests-macrobenchmark.yml @@ -2,28 +2,33 @@ name: 'Integration Tests - Macrobenchmark' # Runs the sentry-uitest-android-macrobenchmark cold-start benchmark on a Sauce Labs real # device and recovers its metrics from the device log. # -# The sample app is built twice -- once from this ref, once from its merge base with main -- and -# both are installed on the device so the benchmark can alternate between them. Absolute numbers -# from a cloud device with unlocked CPU clocks are close to unreadable; a delta measured against -# a baseline on the same device in the same session is not. +# The sample app is built twice -- once from the PR merged into its base, once from the base +# alone -- and both are installed on the device so the benchmark can alternate between them. +# Absolute numbers from a cloud device with unlocked CPU clocks are close to unreadable; a delta +# measured against a baseline on the same device in the same session is not. # +# Opt-in per PR: a full run costs two Gradle builds and up to an hour of a Sauce device at +# concurrency 1, which is far too much to spend on every push. on: - workflow_dispatch: + pull_request: + types: [labeled, synchronize] concurrency: - group: ${{ github.workflow }}-${{ github.ref }} + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} cancel-in-progress: true jobs: macrobenchmark: name: Macrobenchmark runs-on: ubuntu-latest + if: contains(github.event.pull_request.labels.*.name, 'run-macrobenchmark') permissions: contents: read pull-requests: write # to post the comparison back onto the PR # we copy the secret to the env variable in order to access it in the workflow + # Note this is empty for pull requests from forks, which skips every step that needs Sauce. env: SAUCE_USERNAME: ${{ secrets.SAUCE_USERNAME }} @@ -31,18 +36,19 @@ jobs: - name: Git checkout uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - # Full history: the baseline is the merge base with main, which shallow clones can't find. - fetch-depth: 0 - - - name: Resolve the merge base + # This checks out refs/pull/N/merge, the PR already merged into its base, so the + # candidate is what main will actually look like. Depth 2 is enough to resolve its + # first parent below. + fetch-depth: 2 + + # First parent of the merge commit is the base branch tip, which is therefore common to + # both arms: anything that landed on the base since the PR forked is present in each and + # cancels out, leaving the PR's own contribution. + - name: Resolve the base commit id: base - run: | - git fetch --no-tags origin main - echo "sha=$(git merge-base HEAD origin/main)" >> "$GITHUB_OUTPUT" + run: echo "sha=$(git rev-parse HEAD^1)" >> "$GITHUB_OUTPUT" - # Comparing against the merge base rather than main's tip keeps commits that landed on main - # in the meantime out of the delta. - - name: Git checkout the merge base + - name: Git checkout the base commit uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: ref: ${{ steps.base.outputs.sha }} @@ -61,19 +67,33 @@ jobs: if: env.SAUCE_USERNAME != null run: ./gradlew :sentry-samples:sentry-samples-android:assembleRelease :sentry-android-integration-tests:sentry-uitest-android-macrobenchmark:assembleBenchmark - # The init script lives in this checkout, not the baseline one, because the merge base - # predates it. Both builds share a Gradle user home, so this one starts with a warm cache. + # Both builds share a Gradle user home, so this one starts with a warm cache. - name: Assemble the baseline target app if: env.SAUCE_USERNAME != null working-directory: baseline - run: ./gradlew --build-cache :sentry-samples:sentry-samples-android:assembleRelease -I "$GITHUB_WORKSPACE/scripts/baseline-app-id.init.gradle" + run: ./gradlew --build-cache :sentry-samples:sentry-samples-android:assembleRelease -PsampleAppIdSuffix=.baseline - name: Stage the baseline apk for Sauce if: env.SAUCE_USERNAME != null run: | + candidate=sentry-samples/sentry-samples-android/build/outputs/apk/release + base=baseline/$candidate + + # sampleAppIdSuffix has to exist in the *base* commit's build script for the baseline to + # get its own application id. A base predating that support silently builds a second copy + # of the same package, which Sauce would install over the candidate -- and the benchmark + # would then compare an app against itself and report a delta of roughly zero. Fail loudly + # instead. + base_id=$(jq -r .applicationId "$base/output-metadata.json") + candidate_id=$(jq -r .applicationId "$candidate/output-metadata.json") + if [ "$base_id" = "$candidate_id" ]; then + echo "::error::Baseline and candidate share the application id $base_id, so they cannot be installed side by side. The base commit ${{ steps.base.outputs.sha }} is missing the sampleAppIdSuffix property; rebase onto a base that has it." + exit 1 + fi + echo "baseline=$base_id candidate=$candidate_id" + mkdir -p build/macrobenchmark-baseline - cp baseline/sentry-samples/sentry-samples-android/build/outputs/apk/release/sentry-samples-android-release.apk \ - build/macrobenchmark-baseline/ + cp "$base/sentry-samples-android-release.apk" build/macrobenchmark-baseline/ - name: Run Macrobenchmark in SauceLab uses: saucelabs/saucectl-run-action@283660aa934c02723c497efa151d582a3acc5801 # pin@v3 @@ -86,63 +106,26 @@ jobs: config-file: .sauce/sentry-uitest-android-macrobenchmark.yml # Runs even when the suite fails: a failed benchmark still logs whatever it managed to - # measure, and the parser's own error explains what was missing. + # measure, and the parser's own error explains what was missing. summary.md is written only + # once the parser succeeds, so a failed recovery leaves no half-written report to post. - name: Recover benchmark results from the device log if: ${{ !cancelled() && env.SAUCE_USERNAME != null }} run: | - # Without pipefail the step passes on `tee`'s exit code, so a failed recovery - # would report success while silently producing no results. - set -o pipefail + mkdir -p ./artifacts python3 scripts/parse-macrobenchmark-log.py ./artifacts \ --json-out ./artifacts/benchmarkData.json \ --base-sha "${{ steps.base.outputs.sha }}" \ - --head-sha "${{ github.sha }}" \ - | tee ./artifacts/summary.md >> "$GITHUB_STEP_SUMMARY" + --head-sha "${{ github.event.pull_request.head.sha }}" \ + > ./artifacts/summary.md.tmp + mv ./artifacts/summary.md.tmp ./artifacts/summary.md + cat ./artifacts/summary.md >> "$GITHUB_STEP_SUMMARY" - # workflow_dispatch has no PR context, so the PR is looked up from the dispatched branch. - # Dispatching on a branch without an open PR is fine -- the job summary still has the table. - name: Comment the comparison on the PR - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - if: ${{ !cancelled() && env.SAUCE_USERNAME != null }} + uses: marocchino/sticky-pull-request-comment@5770ad5eb8f42dd2c4f34da00c94c5381e49af88 # v3.0.5 + if: ${{ !cancelled() && env.SAUCE_USERNAME != null && hashFiles('./artifacts/summary.md') != '' }} with: - script: | - const fs = require('fs'); - const marker = ''; - - let body; - try { - body = fs.readFileSync('./artifacts/summary.md', 'utf8'); - } catch { - core.info('No summary to post; the benchmark did not report results.'); - return; - } - // The recovery step is allowed to fail, and `tee` leaves a truncated file behind when - // it does. Only a report that got as far as writing its marker is worth posting. - if (!body.startsWith(marker)) { - core.info('Summary is incomplete; leaving the PR comment alone.'); - return; - } - - const head = `${context.repo.owner}:${context.ref.replace('refs/heads/', '')}`; - const { data: prs } = await github.rest.pulls.list({ ...context.repo, head, state: 'open' }); - if (prs.length === 0) { - core.info(`No open PR for ${head}; results are in the job summary only.`); - return; - } - const issue_number = prs[0].number; - - // Update this workflow's own comment rather than stacking a new one on every dispatch. - const comments = await github.paginate(github.rest.issues.listComments, { - ...context.repo, - issue_number, - }); - const existing = comments.find((comment) => comment.body?.startsWith(marker)); - - if (existing) { - await github.rest.issues.updateComment({ ...context.repo, comment_id: existing.id, body }); - } else { - await github.rest.issues.createComment({ ...context.repo, issue_number, body }); - } + header: macrobenchmark + path: ./artifacts/summary.md - name: Upload Sauce artifacts uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 diff --git a/.sauce/sentry-uitest-android-macrobenchmark.yml b/.sauce/sentry-uitest-android-macrobenchmark.yml index 906d275bad1..ac87357a59d 100644 --- a/.sauce/sentry-uitest-android-macrobenchmark.yml +++ b/.sauce/sentry-uitest-android-macrobenchmark.yml @@ -19,7 +19,7 @@ espresso: app: ./sentry-samples/sentry-samples-android/build/outputs/apk/release/sentry-samples-android-release.apk testApp: ./sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/build/outputs/apk/benchmark/sentry-uitest-android-macrobenchmark-benchmark.apk otherApps: - # The merge-base build of the same app, applicationId suffixed `.baseline` so it installs + # The base-commit build of the same app, applicationId suffixed `.baseline` so it installs # alongside the one above. The workflow stages it here; when it is missing, saucectl fails # rather than silently measuring one build. - ./build/macrobenchmark-baseline/sentry-samples-android-release.apk diff --git a/scripts/baseline-app-id.init.gradle b/scripts/baseline-app-id.init.gradle deleted file mode 100644 index 745ca07b039..00000000000 --- a/scripts/baseline-app-id.init.gradle +++ /dev/null @@ -1,20 +0,0 @@ -// Suffixes the sample app's applicationId so a second build of it can be installed alongside the -// one under test. Used by the macrobenchmark workflow to put the merge-base build and the PR -// build on the same device at the same time. -// -// This is injected with `-I` rather than committed to the sample's own build script because the -// baseline is built from a checkout of the merge base, which predates this file. -// -// Groovy, not Kotlin: a Kotlin init script would need AGP on its own `initscript` classpath to -// see the types behind the `android` extension, while Groovy resolves it dynamically. -gradle.beforeProject { project -> - if (project.path != ':sentry-samples:sentry-samples-android') { - return - } - // Registered from `beforeProject`, so it runs ahead of the afterEvaluate AGP registers when the - // build script applies it -- late enough to override whatever the build script set, early - // enough that the DSL is still writable. - project.afterEvaluate { - project.android.defaultConfig.applicationIdSuffix = '.baseline' - } -} diff --git a/scripts/parse-macrobenchmark-log.py b/scripts/parse-macrobenchmark-log.py index 2ff2b8fb6dd..c3fc987f486 100755 --- a/scripts/parse-macrobenchmark-log.py +++ b/scripts/parse-macrobenchmark-log.py @@ -5,7 +5,7 @@ `-benchmarkData.json` into logcat as numbered chunks. This reassembles those chunks and prints a Markdown summary. -When the run alternated between the merge-base build and the build under test, the summary is a +When the run alternated between the base build and the build under test, the summary is a base-vs-PR comparison. When only one build was installed -- a plain local run -- it falls back to reporting that build on its own. @@ -166,7 +166,7 @@ def format_runs_details(label, runs): def format_comparison(data, pooled, base_sha, head_sha): - lines = ["## Macrobenchmark: PR vs merge base", ""] + lines = ["## Macrobenchmark: PR vs base", ""] lines += format_header(data, base_sha, head_sha) if not data["context"]["cpuLocked"]: @@ -253,11 +253,9 @@ def format_summary(data, base_sha=None, head_sha=None): pooled = pool_runs(data["benchmarks"]) if not pooled: sys.exit("Benchmark data contains no results") - # The marker lets the workflow find and update its own PR comment instead of adding another. - marker = "" if BASELINE in pooled and CANDIDATE in pooled: - return f"{marker}\n{format_comparison(data, pooled, base_sha, head_sha)}" - return f"{marker}\n{format_single(data, pooled, base_sha, head_sha)}" + return format_comparison(data, pooled, base_sha, head_sha) + return format_single(data, pooled, base_sha, head_sha) def main(): diff --git a/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/README.md b/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/README.md index 3535494a875..4654548fc67 100644 --- a/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/README.md +++ b/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/README.md @@ -2,7 +2,7 @@ Jetpack Macrobenchmark for cold-start of `sentry-samples-android`, used to evaluate SDK-init performance changes on a real device in a **stable, reproducible** way. Runs on Sauce Labs from the -`Integration Tests - Macrobenchmark` workflow, on a manual trigger only. +`Integration Tests - Macrobenchmark` workflow, on any PR labelled `run-macrobenchmark`. ## What it measures @@ -30,8 +30,11 @@ conditions, so the benchmark measures **two builds of the sample app at once**: | Variant | Package | Built from | |---|---|---| -| `CANDIDATE` | `io.sentry.samples.android` | the ref under test | -| `BASELINE` | `io.sentry.samples.android.baseline` | its merge base with `main` | +| `CANDIDATE` | `io.sentry.samples.android` | the PR merged into its base | +| `BASELINE` | `io.sentry.samples.android.baseline` | the base commit alone | + +Because the candidate is the *merge* of the PR into its base, anything that landed on the base +since the PR forked is present in both arms and cancels out, leaving the PR's own contribution. Both are installed on the same device and the run alternates between them, so whatever the device does over the session — thermal throttling above all — lands on both and cancels out in the @@ -44,9 +47,14 @@ difference. Two things follow from that, and both are deliberate: class of device where 12 back-to-back ones only start to. Read the delta, not the medians either side of it. -The suffixed applicationId comes from `scripts/baseline-app-id.init.gradle`, applied with `-I` when -building the baseline. It is injected rather than committed to the sample's build script because -the baseline is built from a checkout of the merge base, which predates the file. +The suffixed applicationId comes from the `sampleAppIdSuffix` Gradle property, which +`sentry-samples-android` reads in its `defaultConfig`. The baseline build passes +`-PsampleAppIdSuffix=.baseline`; every ordinary build leaves it unset. + +That property has to exist in the **base** commit's build script, since that is what the baseline +is built from. Against a base predating it, both builds produce the same package and the workflow +fails with an explicit error rather than installing one over the other and comparing the app +against itself. If only the candidate is installed, the baseline steps are **skipped** (not failed) and the report falls back to single-build numbers. @@ -62,13 +70,14 @@ Connect a device, then: Results print to the console and are written to `build/outputs/connected_android_test_additional_output/.../*-benchmarkData.json`. -That measures the candidate only. To get the comparison locally, build the baseline out of a -worktree at the merge base and install it alongside: +That measures the candidate only. To get the comparison locally, build a second copy of the app +with the suffix and install it alongside. Point the worktree at whatever you want as the baseline +— the merge base, or just `origin/main`: ```bash -git worktree add ../baseline "$(git merge-base HEAD origin/main)" +git worktree add ../baseline origin/main (cd ../baseline && ./gradlew :sentry-samples:sentry-samples-android:assembleRelease \ - -I "$OLDPWD/scripts/baseline-app-id.init.gradle") + -PsampleAppIdSuffix=.baseline) adb install -r ../baseline/sentry-samples/sentry-samples-android/build/outputs/apk/release/sentry-samples-android-release.apk ``` @@ -90,10 +99,16 @@ the same base-vs-PR table can be produced from a local run. ## Running on Sauce Labs -The `Integration Tests - Macrobenchmark` workflow (manual trigger) runs the same benchmark on a -Sauce Labs real device: it builds the sample app from the dispatched ref and from its merge base, -ships both, and posts the comparison to the job summary and to a PR comment it keeps updating. -It reports numbers only and never fails on them. +The `Integration Tests - Macrobenchmark` workflow runs the same benchmark on a Sauce Labs real +device: it builds the sample app from the PR merge and from the base commit, ships both, and posts +the comparison to the job summary and to a PR comment it keeps updating. It reports numbers only +and never fails on them. + +It runs only on PRs carrying the **`run-macrobenchmark`** label — add the label to start a run, +and subsequent pushes to that PR re-run it. A full run costs two Gradle builds and up to an hour +of a Sauce device at concurrency 1, which is far too much to spend on every push. PRs from forks +get no secrets, so the Sauce steps skip; do not reach for `pull_request_target` to change that, as +it would run a fork's build scripts with our Sauce credentials in scope. Two configuration details in `.sauce/sentry-uitest-android-macrobenchmark.yml` are load-bearing: @@ -127,7 +142,7 @@ local device. ## Known limitations -- **A merge base older than the `SentryAndroid.init` trace section** (added in #5901) has no such +- **A base commit older than the `SentryAndroid.init` trace section** (added in #5901) has no such section for `TraceSectionMetric` to find, and Macrobenchmark fails the whole run rather than reporting the other metric. Rebase onto a newer `main` if you hit this. - Both builds carry the same DSN, so baseline launches also send events to the sample project. diff --git a/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/java/io/sentry/uitest/android/macrobenchmark/SentryStartupBenchmark.kt b/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/java/io/sentry/uitest/android/macrobenchmark/SentryStartupBenchmark.kt index 30cb37675eb..bf5f243c1b1 100644 --- a/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/java/io/sentry/uitest/android/macrobenchmark/SentryStartupBenchmark.kt +++ b/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/src/main/java/io/sentry/uitest/android/macrobenchmark/SentryStartupBenchmark.kt @@ -20,9 +20,7 @@ import org.junit.runners.Parameterized /** The two builds of the sample app a run can measure. */ enum class Variant(val packageName: String) { - /** - * The merge-base build, suffixed by `scripts/baseline-app-id.init.gradle` so both can coexist. - */ + /** The base-commit build, suffixed via the `sampleAppIdSuffix` property so both can coexist. */ BASELINE("io.sentry.samples.android.baseline"), /** The build under test. The only variant present on a plain local run. */ diff --git a/sentry-samples/sentry-samples-android/build.gradle.kts b/sentry-samples/sentry-samples-android/build.gradle.kts index 31009f6dbb9..7e654ac9c37 100644 --- a/sentry-samples/sentry-samples-android/build.gradle.kts +++ b/sentry-samples/sentry-samples-android/build.gradle.kts @@ -47,6 +47,9 @@ android { defaultConfig { applicationId = "io.sentry.samples.android" + // Lets the macrobenchmark workflow build a second copy of this app that installs alongside the + // normal one, so it can cold-start both in a single session. Unset for every ordinary build. + applicationIdSuffix = providers.gradleProperty("sampleAppIdSuffix").orNull // androidx.sqlite 2.6+ require minSdk 23; the Sentry SDK still supports 21. minSdk = 23 targetSdk = libs.versions.targetSdk.get().toInt() From b91db15172396baa958f88227ced0f68b6ec0a1d Mon Sep 17 00:00:00 2001 From: Nelson Osacky Date: Tue, 25 Aug 2026 15:14:39 +0200 Subject: [PATCH 3/3] ci(macrobenchmark): Run the comparison on every pull request (JAVA-679) Drops the run-macrobenchmark label gate, so the benchmark runs unprompted on every PR instead of waiting to be asked. Cost is now carried by the concurrency group rather than by the gate: a new push supersedes that PR's in-flight run instead of queueing behind it at concurrency 1. If the aggregate spend turns out to be too high, a paths filter or a label gate on the job puts it back without touching anything else. --- .../workflows/integration-tests-macrobenchmark.yml | 7 +++---- .../sentry-uitest-android-macrobenchmark/README.md | 14 ++++++++------ 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/.github/workflows/integration-tests-macrobenchmark.yml b/.github/workflows/integration-tests-macrobenchmark.yml index 09c91aece9d..53dd462224f 100644 --- a/.github/workflows/integration-tests-macrobenchmark.yml +++ b/.github/workflows/integration-tests-macrobenchmark.yml @@ -7,11 +7,11 @@ name: 'Integration Tests - Macrobenchmark' # Absolute numbers from a cloud device with unlocked CPU clocks are close to unreadable; a delta # measured against a baseline on the same device in the same session is not. # -# Opt-in per PR: a full run costs two Gradle builds and up to an hour of a Sauce device at -# concurrency 1, which is far too much to spend on every push. +# Runs on every pull request. One run costs two Gradle builds and up to an hour of a Sauce device +# at concurrency 1, so the concurrency group below matters: a new push supersedes the in-flight +# run for that PR rather than queueing behind it. on: pull_request: - types: [labeled, synchronize] concurrency: group: ${{ github.workflow }}-${{ github.event.pull_request.number }} @@ -21,7 +21,6 @@ jobs: macrobenchmark: name: Macrobenchmark runs-on: ubuntu-latest - if: contains(github.event.pull_request.labels.*.name, 'run-macrobenchmark') permissions: contents: read diff --git a/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/README.md b/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/README.md index 4654548fc67..ae4cae65b2b 100644 --- a/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/README.md +++ b/sentry-android-integration-tests/sentry-uitest-android-macrobenchmark/README.md @@ -2,7 +2,7 @@ Jetpack Macrobenchmark for cold-start of `sentry-samples-android`, used to evaluate SDK-init performance changes on a real device in a **stable, reproducible** way. Runs on Sauce Labs from the -`Integration Tests - Macrobenchmark` workflow, on any PR labelled `run-macrobenchmark`. +`Integration Tests - Macrobenchmark` workflow, on every pull request. ## What it measures @@ -104,11 +104,13 @@ device: it builds the sample app from the PR merge and from the base commit, shi the comparison to the job summary and to a PR comment it keeps updating. It reports numbers only and never fails on them. -It runs only on PRs carrying the **`run-macrobenchmark`** label — add the label to start a run, -and subsequent pushes to that PR re-run it. A full run costs two Gradle builds and up to an hour -of a Sauce device at concurrency 1, which is far too much to spend on every push. PRs from forks -get no secrets, so the Sauce steps skip; do not reach for `pull_request_target` to change that, as -it would run a fork's build scripts with our Sauce credentials in scope. +It runs on every pull request. One run costs two Gradle builds and up to an hour of a Sauce device +at concurrency 1, so the workflow's concurrency group cancels a PR's in-flight run when a new push +arrives. If that turns out to be too expensive in aggregate, the cheapest lever is a `paths` filter +or a label gate on the job — neither changes anything else here. + +PRs from forks get no secrets, so the Sauce steps skip. Do not reach for `pull_request_target` to +change that: it would run a fork's build scripts with our Sauce credentials in scope. Two configuration details in `.sauce/sentry-uitest-android-macrobenchmark.yml` are load-bearing: