feat(T7-v14): tasken L62 adoption — record_error on circular-import#58
feat(T7-v14): tasken L62 adoption — record_error on circular-import#58KooshaPari wants to merge 1 commit into
Conversation
…le-3 T7) Adds one `pheno_otel::metrics::record_error` call (gated by the `otel` feature) to the circular-import check in `application::import`, the highest-traffic error site in the import pipeline. Adds `pheno-otel` as an optional path dep behind the same `otel` feature as the existing tracing + opentelemetry deps so non-OTel consumers don't pull it in. Refs: plans/2026-06-20-v14-71-pillar-cycle-3-p0.md § T7, ADR-036B, ADR-042. Depends on: pheno-otel#52 (metrics API).
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
codeant-ai
Bot
added
the
size:XS
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c2a418e24
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # so consumers that don't want OTel don't pull it in. When enabled, the | ||
| # `import::ImportError::CircularImport` site records an `errors.count` metric | ||
| # labelled `tasken.import.circular` so fleet-wide error rate dashboards see it. | ||
| pheno-otel = { path = "../pheno-otel", optional = true } |
There was a problem hiding this comment.
In a clean checkout that only contains Tasken (the CI workflow checks out only this repository before cargo build --workspace), this optional path dependency still makes Cargo load /workspace/pheno-otel/Cargo.toml during manifest resolution; I checked with cargo metadata --no-deps, and it fails with failed to load manifest for dependency pheno-otel before any feature selection or build can proceed. Please vendor/workspace the crate or depend on a published/git source so default builds do not require a sibling directory.
Useful? React with 👍 / 👎.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| // --- Circular import check --- | ||
| if visited.iter().any(|p| p == path) { | ||
| #[cfg(feature = "otel")] | ||
| pheno_otel::metrics::record_error("tasken.import", "circular_import"); |
There was a problem hiding this comment.
Suggestion: The error reason string does not match the documented metric label contract for this integration (tasken.import.circular), so circular-import failures will be recorded under a different series and won't show up in the intended error-rate dashboard. Use the expected reason token so this call emits the same metric key/labels that the observability pipeline is configured to aggregate. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Circular import errors recorded under unexpected metric label.
- ⚠️ L62 error-rate dashboard misses circular import failures.
- ⚠️ Alerts for circular imports may never fire.Steps of Reproduction ✅
1. Enable the `otel` feature for the Tasken crate so the optional observability
dependencies are active (see `Cargo.toml:23-32`, where the `otel` feature wires in
`dep:pheno-otel` and is documented as L62 error-rate observability).
2. Observe the documented expectation in `Cargo.toml:8-12` that the
`import::ImportError::CircularImport` site "records an `errors.count` metric labelled
`tasken.import.circular` so fleet-wide error rate dashboards see it."
3. Run the circular-import test with the `otel` feature enabled, e.g. `cargo test
--features otel test_import_cycle_detected` which exercises `ImportResolver::resolve_file`
in `src/application/import.rs:129` and the cycle-detection test
`test_import_cycle_detected` at `src/application/import.rs:521-527` (this constructs
`a.json` and `b.json` that import each other and calls `resolver.resolve_file(&a_path)`).
4. When the cycle is detected, `resolve_inner` in `src/application/import.rs:12-19` (file
lines ~140-179) executes the circular-import guard; at line 155 it calls
`pheno_otel::metrics::record_error("tasken.import", "circular_import");`, which produces a
metric label based on `"tasken.import.circular_import"` instead of the documented
`"tasken.import.circular"` token, so circular-import errors are emitted under a different
label than the dashboards are configured to aggregate.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/application/import.rs
**Line:** 155:155
**Comment:**
*Api Mismatch: The error reason string does not match the documented metric label contract for this integration (`tasken.import.circular`), so circular-import failures will be recorded under a different series and won't show up in the intended error-rate dashboard. Use the expected reason token so this call emits the same metric key/labels that the observability pipeline is configured to aggregate.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Code Review SummaryStatus: 2 Issues Found | Recommendation: Request Changes Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 127K · Output: 11.7K · Cached: 172.4K |
1 participant
User description
L62 (error rate observability) adoption for v14 cycle-3 T7. Adds one
pheno_otel::metrics::record_errorcall (gated by theotelfeature) toapplication::importcircular-import check. Addspheno-otelas optional path dep behind the sameotelfeature. Depends on pheno-otel#52.CodeAnt-AI Description
Record circular import errors in observability when OTel is enabled
What Changed
otelfeature, so non-OTel builds are unchangedImpact
✅ Clearer import error dashboards✅ Faster detection of circular-import spikes✅ No extra observability dependency for non-OTel users💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.