Skip to content

feat(T7-v14): tasken L62 adoption — record_error on circular-import #58

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.

Sign up for GitHub

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

Open
KooshaPari wants to merge 1 commit into
base: main
Choose a base branch
from
+156 −4
Open

feat(T7-v14): tasken L62 adoption — record_error on circular-import #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 147 additions & 3 deletions Cargo.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,12 @@ dependencies = [
"libc",
]

[[package]]
name = "lazy_static"
version = "1.5.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "bbd2bcb4c963f2ddae06a2efc7e9f3591312473c50c6685e1f298068316e66fe"

[[package]]
name = "leb128fmt"
version = "0.1.0"
Expand Down Expand Up @@ -603,6 +609,15 @@ dependencies = [
"bitflags",
]

[[package]]
name = "nu-ansi-term"
version = "0.50.3"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5"
dependencies = [
"windows-sys 0.61.2",
]

[[package]]
name = "num-traits"
version = "0.2.19"
Expand All @@ -624,6 +639,20 @@ version = "1.70.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "384b8ab6d37215f3c5301a95a4accb5d64aa607f1fcb26a11b5303878451b4fe"

[[package]]
name = "opentelemetry"
version = "0.24.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "4c365a63eec4f55b7efeceb724f1336f26a9cf3427b70e59e2cd2a5b947fba96"
dependencies = [
"futures-core",
"futures-sink",
"js-sys",
"once_cell",
"pin-project-lite",
"thiserror 1.0.69",
]

[[package]]
name = "option-ext"
version = "0.2.0"
Expand Down Expand Up @@ -665,6 +694,16 @@ dependencies = [
"serde",
]

[[package]]
name = "pheno-otel"
version = "0.1.0"
dependencies = [
"opentelemetry",
"serde",
"serde_json",
"thiserror 2.0.18",
]

[[package]]
name = "pin-project-lite"
version = "0.2.17"
Expand Down Expand Up @@ -722,7 +761,7 @@ checksum = "a4e608c6638b9c18977b00b475ac1f28d14e84b27d8d42f70e0bf1e3dec127ac"
dependencies = [
"getrandom 0.2.17",
"libredox",
"thiserror",
"thiserror 2.0.18",
]

[[package]]
Expand Down Expand Up @@ -836,6 +875,15 @@ dependencies = [
"unsafe-libyaml",
]

[[package]]
name = "sharded-slab"
version = "0.1.7"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "f40ca3c46823713e0d4209592e8d6e826aa57e928f09752619fc696c499637f6"
dependencies = [
"lazy_static",
]

[[package]]
name = "shlex"
version = "2.0.1"
Expand Down Expand Up @@ -906,15 +954,19 @@ dependencies = [
"futures",
"libc",
"notify",
"opentelemetry",
"petgraph",
"pheno-otel",
"serde",
"serde_json",
"serde_yaml",
"temp-env",
"tempfile",
"thiserror",
"thiserror 2.0.18",
"tokio",
"toml",
"tracing",
"tracing-subscriber",
"uuid",
]

Expand All @@ -940,13 +992,33 @@ dependencies = [
"windows-sys 0.61.2",
]

[[package]]
name = "thiserror"
version = "1.0.69"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "b6aaf5339b578ea85b50e080feb250a3e8ae8cfcdff9a461c9ec2904bc923f52"
dependencies = [
"thiserror-impl 1.0.69",
]

[[package]]
name = "thiserror"
version = "2.0.18"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "4288b5bcbc7920c07a1149a35cf9590a2aa808e0bc1eafaade0b80947865fbc4"
dependencies = [
"thiserror-impl",
"thiserror-impl 2.0.18",
]

[[package]]
name = "thiserror-impl"
version = "1.0.69"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "4fee6c4efc90059e10f81e6d42c60a18f76588c3d74cb83a0b242a2b6c7504c1"
dependencies = [
"proc-macro2",
"quote",
"syn",
]

[[package]]
Expand All @@ -960,6 +1032,15 @@ dependencies = [
"syn",
]

[[package]]
name = "thread_local"
version = "1.1.9"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "f60246a4944f24f6e018aa17cdeffb7818b76356965d03b07d6a9886e8962185"
dependencies = [
"cfg-if",
]

[[package]]
name = "tokio"
version = "1.52.3"
Expand Down Expand Up @@ -1029,6 +1110,63 @@ version = "0.1.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "5d99f8c9a7727884afe522e9bd5edbfc91a3312b36a77b5fb8926e4c31a41801"

[[package]]
name = "tracing"
version = "0.1.44"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "63e71662fa4b2a2c3a26f570f037eb95bb1f85397f3cd8076caed2f026a6d100"
dependencies = [
"pin-project-lite",
"tracing-attributes",
"tracing-core",
]

[[package]]
name = "tracing-attributes"
version = "0.1.31"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "7490cfa5ec963746568740651ac6781f701c9c5ea257c58e057f3ba8cf69e8da"
dependencies = [
"proc-macro2",
"quote",
"syn",
]

[[package]]
name = "tracing-core"
version = "0.1.36"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "db97caf9d906fbde555dd62fa95ddba9eecfd14cb388e4f491a66d74cd5fb79a"
dependencies = [
"once_cell",
"valuable",
]

[[package]]
name = "tracing-log"
version = "0.2.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "ee855f1f400bd0e5c02d150ae5de3840039a3f54b025156404e34c23c03f47c3"
dependencies = [
"log",
"once_cell",
"tracing-core",
]

[[package]]
name = "tracing-subscriber"
version = "0.3.23"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "cb7f578e5945fb242538965c2d0b04418d38ec25c79d160cd279bf0731c8d319"
dependencies = [
"nu-ansi-term",
"sharded-slab",
"smallvec",
"thread_local",
"tracing-core",
"tracing-log",
]

[[package]]
name = "unicode-ident"
version = "1.0.24"
Expand Down Expand Up @@ -1065,6 +1203,12 @@ dependencies = [
"wasm-bindgen",
]

[[package]]
name = "valuable"
version = "0.1.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "ba73ea9cf16a25df0c8caa16c51acb937d5712a8429db78a3ee29d5dcacd3a65"

[[package]]
name = "walkdir"
version = "2.5.0"
Expand Down
8 changes: 7 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,12 @@ serde_yaml = "0.9"
# Observability (optional, gated by the `otel` feature flag)
tracing = { version = "0.1", optional = true }
opentelemetry = { version = "0.24", optional = true }
# L62 (error rate) observability adoption (v14 cycle-3 T7).
# Optional, gated by the same `otel` feature as the other observability deps
# 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 }

@chatgpt-codex-connector chatgpt-codex-connector Bot Jun 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid requiring an external sibling crate for every build

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 👍 / 👎.


[dev-dependencies]
tempfile = "3"
Expand All @@ -72,4 +78,4 @@ default = []
# is wired into TaskService::run_task and TaskService::execute_workflow,
# but the actual exporter (stdout, OTLP, etc.) is left to the embedding
# application — see `infrastructure::otel::init_tracer` for a stub.
otel = ["dep:tracing", "dep:opentelemetry"]
otel = ["dep:tracing", "dep:opentelemetry", "dep:pheno-otel"]
2 changes: 2 additions & 0 deletions src/application/import.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,8 @@ impl ImportResolver {
) -> Result<ImportResult, ImportError> {
// --- Circular import check ---
if visited.iter().any(|p| p == path) {
#[cfg(feature = "otel")]
pheno_otel::metrics::record_error("tasken.import", "circular_import");

@codeant-ai codeant-ai Bot Jun 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in VSCode Claude

(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
👍 | 👎

return Err(ImportError::CircularImport {
path: path.to_path_buf(),
});
Expand Down
Loading