-
Notifications
You must be signed in to change notification settings - Fork 94
feat: panic and sentry hook for foundations #160
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
base: main
Are you sure you want to change the base?
Conversation
This commit adds a simple AGENTS.md. This wasn't human-curated, just the output of opencode's `/init` with Opus 4.5.
.github/workflows/ci.yml
Fixed
| name: Build | ||
| runs-on: ${{ matrix.os }} | ||
| strategy: | ||
| matrix: | ||
| thing: | ||
| - x86_64-linux | ||
| - aarch64-linux | ||
| - arm64-android | ||
| - arm-android | ||
| - aarch64-ios | ||
| - x86_64-macos | ||
| - x86_64-windows | ||
| include: | ||
| - apt_packages: "" | ||
| - custom_env: {} | ||
| - build_only: false | ||
| - cargo_args: "" | ||
|
|
||
| - thing: x86_64-linux | ||
| target: x86_64-unknown-linux-gnu | ||
| rust: stable | ||
| os: ubuntu-latest | ||
|
|
||
| - thing: aarch64-linux | ||
| build_only: true | ||
| target: aarch64-unknown-linux-gnu | ||
| rust: stable | ||
| os: ubuntu-latest | ||
| apt_packages: crossbuild-essential-arm64 | ||
| custom_env: | ||
| CC: aarch64-linux-gnu-gcc | ||
| CXX: aarch64-linux-gnu-g++ | ||
| CARGO_TARGET_AARCH64_UNKNOWN_LINUX_GNU_LINKER: aarch64-linux-gnu-g++ | ||
|
|
||
| - thing: arm64-android | ||
| build_only: true | ||
| target: aarch64-linux-android | ||
| rust: stable | ||
| os: ubuntu-latest | ||
| cargo_args: --no-default-features --features server-client-common-default | ||
|
|
||
| - thing: arm-android | ||
| build_only: true | ||
| target: armv7-linux-androideabi | ||
| rust: stable | ||
| os: ubuntu-latest | ||
| cargo_args: --no-default-features --features server-client-common-default | ||
|
|
||
| - thing: aarch64-ios | ||
| build_only: true | ||
| target: aarch64-apple-ios | ||
| rust: stable | ||
| os: macos-latest | ||
| cargo_args: --no-default-features --features server-client-common-default | ||
|
|
||
| - thing: x86_64-macos | ||
| target: x86_64-apple-darwin | ||
| rust: stable | ||
| os: macos-latest | ||
|
|
||
| - thing: x86_64-windows | ||
| build_only: true | ||
| target: x86_64-pc-windows-msvc | ||
| rust: stable | ||
| os: windows-latest | ||
| cargo_args: --no-default-features --features server-client-common-default | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| submodules: "recursive" | ||
| - name: Install Rust | ||
| uses: dtolnay/rust-toolchain@master | ||
| with: | ||
| toolchain: ${{ matrix.rust }} | ||
| targets: ${{ matrix.target }} | ||
| - uses: Swatinem/rust-cache@v2 | ||
| - name: Install target-specific APT dependencies | ||
| if: "matrix.apt_packages != ''" | ||
| run: sudo apt update && sudo apt install -y ${{ matrix.apt_packages }} | ||
| shell: bash | ||
| - name: Set Android Linker path | ||
| if: endsWith(matrix.thing, '-android') | ||
| run: echo "CARGO_TARGET_$(echo ${{ matrix.target }} | tr \\-a-z _A-Z)_LINKER=$ANDROID_NDK/toolchains/llvm/prebuilt/linux-x86_64/bin/$(echo ${{ matrix.target }} | sed s/armv7/armv7a/)21-clang++" >> "$GITHUB_ENV" | ||
| - name: Build tests | ||
| # We `build` because we want the linker to verify we are cross-compiling correctly for check-only targets. | ||
| run: cargo build --target ${{ matrix.target }} ${{matrix.cargo_args}} | ||
| - name: Build | ||
| run: cargo build -p foundations --target ${{ matrix.target }} ${{matrix.cargo_args}} | ||
| shell: bash | ||
| env: ${{ matrix.custom_env }} | ||
| - name: Run tests | ||
| if: "!matrix.build_only" | ||
| run: _RJEM_MALLOC_CONF=prof:true cargo test --target ${{ matrix.target }} ${{matrix.cargo_args}} | ||
|
|
||
| test: | ||
| name: Test | ||
| runs-on: ${{ matrix.os }} | ||
| strategy: | ||
| matrix: | ||
| thing: | ||
| - x86_64-linux | ||
| - x86_64-macos | ||
| include: | ||
| - thing: x86_64-linux | ||
| target: x86_64-unknown-linux-gnu | ||
| rust: stable | ||
| os: ubuntu-latest | ||
|
|
||
| - thing: x86_64-macos | ||
| target: x86_64-apple-darwin | ||
| rust: stable | ||
| os: macos-latest | ||
|
|
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To address the Workflow does not contain permissions error, add an explicit permissions key setting minimal required permission to the workflow. Since the jobs shown merely check out code, build, and test, the job(s) only need contents: read; neither push nor PR write permissions are evident from the code.
Best Practice: Add the permissions: contents: read block at the top level of the workflow, immediately below the workflow name and before on:, so that it applies to all jobs unless overridden.
Steps:
- Edit
.github/workflows/ci.yml. - Add the following at line 2:
permissions: contents: read
- This ensures that all jobs use the minimum
GITHUB_TOKENprivilege.
-
Copy modified lines R2-R3
| @@ -1,4 +1,6 @@ | ||
| name: CI | ||
| permissions: | ||
| contents: read | ||
|
|
||
| on: | ||
| pull_request: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
f3e444a to
3773226
Compare
9203b72 to
5887224
Compare
| #[cfg(feature = "metrics")] | ||
| self::metrics::init::init(config.service_info, &config.settings.metrics); | ||
|
|
||
| let _ = TELEMETRY_INITIALIZED.set(()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if there's a better way to do this
5887224 to
6ab4caf
Compare
| .unwrap_or_else(|| "<unknown>".to_string()); | ||
|
|
||
| // Use serde_json for proper JSON escaping of all control characters | ||
| let json_output = serde_json::json!({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used for escaping the input payload. A little heavy, but most foundations users will likely have serde_json in their dep tree.
6ab4caf to
d19e279
Compare
This PR adds a panic and sentry hook to foundations. The panic hook will increment a metric and log the fact a panic occurred, where the sentry hook only increments a metric. The panic hook uses foundations' logging functionality if it's initialized, otherwise, it will print a JSON formatted log to stderr.
The two metrics emitted by foundations are:
panics_totalandsentry_events_total.To easily test foundations' global metrics, I CI migrated to
cargo-nextest. Also, I moved the target build testing to its own job (just to separate build/test). Finally, I generated a simpleAGENTS.mdwhich we can start refining to help with future PRs.The diff looks rather large, but the vast majority of lines changed are tests and doc comments. Basically, we add some builder helpers under
alerts::_privateand use those to setup the panic and sentry hook.