Skip to content

Optimize CI caching and metrics flush - #265

Open
YanagiKH wants to merge 1 commit into
pathwaycom:mainfrom
YanagiKH:main
Open

Optimize CI caching and metrics flush#265
YanagiKH wants to merge 1 commit into
pathwaycom:mainfrom
YanagiKH:main

Conversation

@YanagiKH

@YanagiKH YanagiKH commented Aug 1, 2026

Copy link
Copy Markdown

Context

This change reduces repeated CI build time and removes an avoidable allocation in a hot telemetry flush path.

  • The package test workflow now uses Rust caching so unchanged dependencies and build artifacts can be reused across runs.
  • The SQLite metrics exporter now uses std::mem::take when flushing the buffered samples, which avoids draining into a newly allocated vector.

How this change came about

I reviewed the package test workflow and the SQLite telemetry exporter, then made two focused changes:

  1. Added Rust cache to .github/workflows/package_test.yml to reduce repeated build work in CI.
  2. Replaced drain(..).collect() with std::mem::take in src/engine/telemetry/exporter.rs to avoid an extra allocation while flushing buffered telemetry.

I made the edits directly in the repository and kept the scope limited to the two files above.

How has this been tested?

I verified the change by code inspection. No additional runtime test was run in this session.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature or improvement (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation-only change

Checklist:

  • I have read [CONTRIBUTING.md](https://github.com/pathwaycom/pathway/blob/main/CONTRIBUTING.md),
  • My code follows the code style of this project,
  • If this is a substantial change, it has prior maintainer approval and the "How this change came about" section is filled in,
  • I described the modification in the CHANGELOG.md file,
  • My change requires a change to the documentation, and I have updated it accordingly.

- Add Rust cache to the package test workflow to reduce repeated build time.
- Replace the SQLite metrics drain/collect path with `std::mem::take` to avoid an extra allocation when flushing buffered telemetry.
@CLAassistant

CLAassistant commented Aug 1, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@zxqfd555 zxqfd555 self-assigned this Aug 3, 2026
@zxqfd555

zxqfd555 commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the contribution. Before we can consider the CI change, we'd need evidence that the cache actually helps in this specific workflow:

  1. The Linux build runs via maturin-action inside a manylinux container, so the host's ~/.cargo that Swatinem/rust-cache caches is not visible to the cargo that does the actual compilation. Only ./target in the mounted workspace could potentially be reused, and the cache key is derived from the host toolchain, not the container's.
  2. The "Set package version" step rewrites the version in Cargo.toml on every run, so the pathway crate itself is always rebuilt regardless of caching.

Could you run this workflow in your fork twice (so the second run hits the cache) and link both runs, showing an actual wall-clock improvement and a cache restore in the rust-cache step logs? Note the workflow is workflow_call-only, so you'll need a small wrapper workflow to trigger it.

As for the mem::take change in exporter.rs: it trades one allocation per flush for losing the buffer's capacity (subsequent pushes regrow it), and this is a periodic telemetry flush dominated by SQLite inserts, so the net effect is neutral — please drop it from this PR so the PR stays focused on one thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants