Maintenance; static tracker#46
Open
AndyGura wants to merge 13 commits into
Open
Conversation
Co-authored-by: Junie <junie@jetbrains.com>
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces release 0.0.7, consolidating CI/CD workflows, refactoring the StreamTracker metrics API from PromTracker-based to Tracker/Renderer pattern, and updating dependencies across the ecosystem. Examples are migrated to use the new API, legacy documentation is removed, and test syntax is modernized. ChangesRelease 0.0.7 Core Changes
Sequence Diagram(s)sequenceDiagram
participant ExampleActor as Example Actor
participant Tracker
participant Renderer
participant StreamTracker
participant StreamReceiver
ExampleActor->>Tracker: Tracker.new()
ExampleActor->>Renderer: PT.Renderer()
Renderer->>Tracker: addValue(metrics.toValue())
ExampleActor->>StreamTracker: StreamTracker.Receiver(tracker, renderer, labels)
ExampleActor->>StreamReceiver: StreamReceiver.new(...)
StreamTracker->>StreamReceiver: init(receiver)
ExampleActor->>Renderer: include Http(renderer.renderExposition, "/metrics")
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
AndyGura
changed the title
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/TEST_SUMMARY.md (1)
75-77:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate tracker test summary to reflect the new Tracker/Renderer API.
These lines still describe legacy PromTracker/stable-flag behavior, but this release migrated tests to
Tracker.new() + PT.Renderer()and removed the stable-metrics-flag block. The summary should match the current test architecture.Also applies to: 214-214
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/TEST_SUMMARY.md` around lines 75 - 77, The test summary still mentions legacy PromTracker and the stable-metrics-flag; update the three bullet lines to describe the current Tracker/Renderer API (e.g., reference Tracker.new() and PT.Renderer() usage and custom label behavior) and remove or reword any mention of PromTracker or stable-metrics-flag so the summary accurately reflects the new test architecture and behaviors validated by the tests.test/coderabbit/tracker.test.mo (1)
119-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDispose assertions are currently non-validating.
assert expositionAfter.size() >= 0is always true, so these checks won’t catch regressions in cleanup behavior.Proposed assertion tightening
- assert expositionAfter.size() >= 0; + assert expositionAfter.size() <= expositionBefore.size();Also applies to: 302-305
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/coderabbit/tracker.test.mo` around lines 119 - 123, The current assertion uses assert expositionAfter.size() >= 0 which is vacuous; change the test to capture the exposition before disposal (e.g. let expositionBefore = metrics.renderExposition()), call metrics.dispose() (or the existing dispose call), then assert that expositionAfter.size() <= expositionBefore.size() (and optionally assert expositionAfter.size() < expositionBefore.size() when you expect at least one entry removed) to ensure cleanup actually reduced or did not increase metrics; make the same replacement for the other identical assertions mentioned (the block around the later occurrence).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 1-9: The workflow named "CI" lacks an explicit permissions block;
add a top-level permissions section to restrict the default GITHUB_TOKEN scope
(e.g., set contents: read) so the job tokens run with least privilege; update
the YAML near the workflow header (where "name: CI" and "on:" are defined) to
include a permissions: block with at minimum "contents: read" (and any other
minimal permissions required by your jobs).
- Around line 14-15: Replace mutable GitHub Actions tags with immutable commit
SHAs for each `uses:` reference: change `actions/checkout@v6`,
`actions/setup-node@v6`, and `caffeinelabs/setup-mops@v1` to their respective
full 40-character commit SHA pins (e.g., `actions/checkout@<40-char-sha>`),
optionally appending a human-readable `# v6` or `# v1` comment for reference;
update the `uses:` entries where they appear so the workflow uses fixed SHAs
instead of floating tags.
- Line 17: CI uses unpinned Node and formatter packages which makes runs
non-deterministic; update the workflow to pin node-version from "latest" to a
specific LTS (recommend "24.x" or "22.x") for both occurrences and pin the
formatter installs from unpinned packages to exact versions by installing
prettier@3.8.3 and prettier-plugin-motoko@0.13.0 in the formatter step so the
Prettier/Motoko plugin behavior is stable.
- Line 14: The CI workflow uses actions/checkout@v6 which by default persists
credentials; update each checkout step (the occurrences of actions/checkout@v6)
to include persist-credentials: false so the runner does not write the auth
token/SSH key into local Git config — modify both checkout usages found in the
workflow to add the persist-credentials: false input under the uses entry.
- Line 20: Replace the unsafe "curl ... | sh" invocation that downloads and
executes icp-cli-installer.sh directly by changing the CI step that runs the
curl command to instead: download the installer to a file (reference
icp-cli-installer.sh), fetch and verify a published checksum or signature (e.g.,
.sha256/.asc) using a known-good key or checksum file, only then make the
downloaded file executable and run it; update the workflow step name/description
accordingly and fail the job if verification fails so the CI step that currently
contains the curl pipe is no longer executing unverified remote code.
In `@CHANGELOG.md`:
- Line 13: Update the stale changelog entry in CHANGELOG.md that currently says
"- Fixed compiler warnings in `src/Tracker.mo`" to reference the correct file
name `src/StreamTracker.mo`; replace the backticked path `src/Tracker.mo` with
`src/StreamTracker.mo` so the note points to the existing source file.
In `@examples/promtracker/src/sender.mo`:
- Around line 51-58: The sender exposition is missing because the tracker
metrics (pt) were never registered with the renderer; after creating pt
(Tracker.new()) and obtaining the renderer instance (Renderer()) call
renderer.addValue(pt.toValue()) so the gauges/counters that StreamTracker.Sender
registers on pt are exposed; ensure this call happens before including or
serving renderExposition and before tracker.init(sender) so renderer has the pt
values available.
In `@README.md`:
- Around line 120-123: The README usage examples mix import styles; update all
example import statements so they consistently use the package-style imports
(e.g., import StreamSender "mo:stream/StreamSender" and import StreamReceiver
"mo:stream/StreamReceiver") instead of relative paths—replace the relative
imports used in the sender/receiver examples (the blocks that reference
StreamSender and StreamReceiver around the shown sections) with the mo:stream/*
form so copy-paste from the docs works outside the repo layout.
In `@test/coderabbit/tracker.test.mo`:
- Around line 348-350: The test currently only checks that exposition from
metrics.renderExposition() is non-empty; change the assertions to also verify
the custom label is present by asserting the rendered exposition string includes
the literal custom_label="value" (e.g., convert exposition to string via
exposition.toString() or exposition.render() as appropriate) for the existing
check around variable exposition and similarly update the other occurrence at
lines 368-370 so both tests assert the presence of custom_label="value" in the
rendered output.
---
Outside diff comments:
In `@test/coderabbit/tracker.test.mo`:
- Around line 119-123: The current assertion uses assert expositionAfter.size()
>= 0 which is vacuous; change the test to capture the exposition before disposal
(e.g. let expositionBefore = metrics.renderExposition()), call metrics.dispose()
(or the existing dispose call), then assert that expositionAfter.size() <=
expositionBefore.size() (and optionally assert expositionAfter.size() <
expositionBefore.size() when you expect at least one entry removed) to ensure
cleanup actually reduced or did not increase metrics; make the same replacement
for the other identical assertions mentioned (the block around the later
occurrence).
In `@test/TEST_SUMMARY.md`:
- Around line 75-77: The test summary still mentions legacy PromTracker and the
stable-metrics-flag; update the three bullet lines to describe the current
Tracker/Renderer API (e.g., reference Tracker.new() and PT.Renderer() usage and
custom label behavior) and remove or reword any mention of PromTracker or
stable-metrics-flag so the summary accurately reflects the new test architecture
and behaviors validated by the tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: da53f7d7-099c-4843-91ce-d95dca6a64ee
📒 Files selected for processing (34)
.github/workflows/ci.yml.github/workflows/pull_request_build.yml.gitignore.prettierrcCHANGELOG.mdREADME.mddocs/StreamReceiver.mddocs/StreamSender.mddocs/Tracker.mddocs/index.mddocs/types.mdexamples/README.mdexamples/main/mops.tomlexamples/main/src/receiver.moexamples/main/src/sender.moexamples/minimal/mops.tomlexamples/minimal/src/alice.moexamples/minimal/src/bob.moexamples/promtracker/mops.tomlexamples/promtracker/src/receiver.moexamples/promtracker/src/sender.momops.tomlsrc/StreamTracker.mosrc/internal/types.motest/A.motest/B.motest/TEST_SUMMARY.mdtest/async.test.motest/coderabbit/edge_cases.test.motest/coderabbit/integration.test.motest/coderabbit/receiver.advanced.test.motest/coderabbit/sender.advanced.test.motest/coderabbit/tracker.test.motest/coderabbit/types.test.mo
💤 Files with no reviewable changes (6)
- docs/StreamReceiver.md
- docs/types.md
- .github/workflows/pull_request_build.yml
- docs/index.md
- docs/StreamSender.md
- docs/Tracker.md
Collaborator
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores