[refactor] The observability state layer moves into @agenta/observability - #5955
[refactor] The observability state layer moves into @agenta/observability#5955ardaerzin wants to merge 1 commit into
Conversation
dosubot
Bot
added
the
size:XL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
refactoring
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (30)
💤 Files with no reviewable changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR moves observability types, state, query logic, and export utilities into ChangesObservability package migration
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: 🟡 Moderate · up to This refactor moves observability state into a shared package, but the current head can reuse annotations from the wrong project and cannot pass frozen dependency installation until the lockfile is regenerated. These issues should be fixed before merging. Sequence Diagram(s)sequenceDiagram
participant OSSProviders
participant bindObservabilityHostAtoms
participant ObservabilityScope
participant ObservabilityControls
participant ObservabilityQueries
participant TraceAPI
OSSProviders->>bindObservabilityHostAtoms: bind host scope and workflow atoms
bindObservabilityHostAtoms->>ObservabilityScope: provide app, route, workflow, and workspace state
ObservabilityControls->>ObservabilityScope: read persisted filter and trace-type scope
ObservabilityQueries->>TraceAPI: execute project-scoped trace and session queries
TraceAPI-->>ObservabilityQueries: return typed pages and normalized attributes
ObservabilityQueries-->>ObservabilityControls: expose observability state
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/packages/agenta-observability/package.json (1)
26-51: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCommit the regenerated
web/pnpm-lock.yaml.The
packages/agenta-observabilityimporter does not include the five manifest entries. Frozen installs fail withERR_PNPM_OUTDATED_LOCKFILE. Runpnpm installfromweband commit the lockfile.Source: Pipeline failures
🧹 Nitpick comments (1)
web/packages/agenta-observability/src/index.ts (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce the new multiline comments.
These comments do not document a bug, race, or ordering constraint. Replace them with one short line, or remove them when the code is self-explanatory.
web/packages/agenta-observability/src/index.ts#L1-L3: Replace this and the repeated three-line section banners with one short section comment or no comment.web/packages/agenta-observability/src/core/traceSpan.ts#L4-L7: Reduce the interface documentation to one short line.web/oss/src/services/tracing/types/index.ts#L1-L2: Remove or reduce the re-export explanation to one short line.web/oss/src/state/observability/useObservability.ts#L1-L6: Reduce the wrapper description to one short line.As per coding guidelines, “Keep in-code comments to at most one short line; use longer comments only for genuinely surprising constraints such as bugs, races, or ordering requirements.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 585ef4fb-4796-4df9-8471-71555da7de3a
📒 Files selected for processing (30)
web/oss/src/components/Filters/Sort.tsxweb/oss/src/lib/Types.tsweb/oss/src/services/tracing/types/index.tsweb/oss/src/state/Providers.tsxweb/oss/src/state/newObservability/index.tsweb/oss/src/state/observability/atoms.tsweb/oss/src/state/observability/index.tsweb/oss/src/state/observability/seams.tsweb/oss/src/state/observability/useObservability.tsweb/packages/agenta-observability/package.jsonweb/packages/agenta-observability/src/api/queryHelpers.tsweb/packages/agenta-observability/src/core/constants.tsweb/packages/agenta-observability/src/core/traceSpan.tsweb/packages/agenta-observability/src/core/types.tsweb/packages/agenta-observability/src/etl/adaptiveExportPacing.tsweb/packages/agenta-observability/src/etl/adaptiveTracePageFetcher.tsweb/packages/agenta-observability/src/etl/exportWriter.tsweb/packages/agenta-observability/src/etl/withRateLimitRetry.tsweb/packages/agenta-observability/src/hooks/useObservability.tsweb/packages/agenta-observability/src/hooks/useSessions.tsweb/packages/agenta-observability/src/index.tsweb/packages/agenta-observability/src/state/controls.tsweb/packages/agenta-observability/src/state/queries.tsweb/packages/agenta-observability/src/state/seams.tsweb/packages/agenta-observability/src/state/selectors.tsweb/packages/agenta-observability/src/utils/buildTraceQueryParams.tsweb/packages/agenta-observability/src/utils/filterCoercion.tsweb/packages/agenta-observability/tests/unit/queryHelpers.test.tsweb/packages/agenta-observability/tsconfig.jsonweb/packages/agenta-observability/vitest.config.ts
💤 Files with no reviewable changes (1)
- web/oss/src/state/newObservability/index.ts
| queryKey: ["annotations", links], | ||
| queryFn: async () => { | ||
| if (Array.isArray(links) && !links.length) return [] as AnnotationDto[] | ||
| const res = await queryAllAnnotations({annotation: {links}}) | ||
| const res = await queryAllAnnotations({ | ||
| projectId: projectId ?? undefined, | ||
| queries: {annotation: {links}}, | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include projectId in the annotations query key.
queryFn scopes queryAllAnnotations by projectId, but Line 177 omits it from queryKey. A project change with the same links can reuse annotations cached for the prior project. Add projectId to the key.
Proposed fix
- queryKey: ["annotations", links],
+ queryKey: ["annotations", projectId, links],As per coding guidelines, include all reactive dependencies in queryKey.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| queryKey: ["annotations", links], | |
| queryFn: async () => { | |
| if (Array.isArray(links) && !links.length) return [] as AnnotationDto[] | |
| const res = await queryAllAnnotations({annotation: {links}}) | |
| const res = await queryAllAnnotations({ | |
| projectId: projectId ?? undefined, | |
| queries: {annotation: {links}}, | |
| }) | |
| queryKey: ["annotations", projectId, links], | |
| queryFn: async () => { | |
| if (Array.isArray(links) && !links.length) return [] as AnnotationDto[] | |
| const res = await queryAllAnnotations({ | |
| projectId: projectId ?? undefined, | |
| queries: {annotation: {links}}, | |
| }) |
Source: Coding guidelines
| const ag = agOf(trace) | ||
| const tokens = | ||
| (attrs as any)?.ag?.metrics?.tokens?.incremental?.total || | ||
| (attrs as any)?.ag?.metrics?.tokens?.cumulative?.total || | ||
| ag?.metrics?.tokens?.incremental?.total || | ||
| ag?.metrics?.tokens?.cumulative?.total || |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve zero-valued metrics.
|| treats 0 as missing. If an incremental metric is 0 and a cumulative metric exists, the session total uses the cumulative value. Use ?? for these fallback chains.
Proposed fix
- ag?.metrics?.tokens?.incremental?.total ||
- ag?.metrics?.tokens?.cumulative?.total ||
- (attrs["ag.usage.total_tokens"] as number) ||
- (attrs["total_tokens"] as number) ||
+ ag?.metrics?.tokens?.incremental?.total ??
+ ag?.metrics?.tokens?.cumulative?.total ??
+ (attrs["ag.usage.total_tokens"] as number) ??
+ (attrs["total_tokens"] as number) ??
0
...
- ag?.metrics?.costs?.incremental?.total || ag?.metrics?.costs?.cumulative?.total || 0
+ ag?.metrics?.costs?.incremental?.total ??
+ ag?.metrics?.costs?.cumulative?.total ??
+ 0Also applies to: 611-613
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/packages/agenta-observability/package.json (1)
26-51: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCommit the regenerated
web/pnpm-lock.yaml.The
packages/agenta-observabilityimporter does not include the five manifest entries. Frozen installs fail withERR_PNPM_OUTDATED_LOCKFILE. Runpnpm installfromweband commit the lockfile.Source: Pipeline failures
🧹 Nitpick comments (1)
web/packages/agenta-observability/src/index.ts (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce the new multiline comments.
These comments do not document a bug, race, or ordering constraint. Replace them with one short line, or remove them when the code is self-explanatory.
web/packages/agenta-observability/src/index.ts#L1-L3: Replace this and the repeated three-line section banners with one short section comment or no comment.web/packages/agenta-observability/src/core/traceSpan.ts#L4-L7: Reduce the interface documentation to one short line.web/oss/src/services/tracing/types/index.ts#L1-L2: Remove or reduce the re-export explanation to one short line.web/oss/src/state/observability/useObservability.ts#L1-L6: Reduce the wrapper description to one short line.As per coding guidelines, “Keep in-code comments to at most one short line; use longer comments only for genuinely surprising constraints such as bugs, races, or ordering requirements.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 585ef4fb-4796-4df9-8471-71555da7de3a
📒 Files selected for processing (30)
web/oss/src/components/Filters/Sort.tsxweb/oss/src/lib/Types.tsweb/oss/src/services/tracing/types/index.tsweb/oss/src/state/Providers.tsxweb/oss/src/state/newObservability/index.tsweb/oss/src/state/observability/atoms.tsweb/oss/src/state/observability/index.tsweb/oss/src/state/observability/seams.tsweb/oss/src/state/observability/useObservability.tsweb/packages/agenta-observability/package.jsonweb/packages/agenta-observability/src/api/queryHelpers.tsweb/packages/agenta-observability/src/core/constants.tsweb/packages/agenta-observability/src/core/traceSpan.tsweb/packages/agenta-observability/src/core/types.tsweb/packages/agenta-observability/src/etl/adaptiveExportPacing.tsweb/packages/agenta-observability/src/etl/adaptiveTracePageFetcher.tsweb/packages/agenta-observability/src/etl/exportWriter.tsweb/packages/agenta-observability/src/etl/withRateLimitRetry.tsweb/packages/agenta-observability/src/hooks/useObservability.tsweb/packages/agenta-observability/src/hooks/useSessions.tsweb/packages/agenta-observability/src/index.tsweb/packages/agenta-observability/src/state/controls.tsweb/packages/agenta-observability/src/state/queries.tsweb/packages/agenta-observability/src/state/seams.tsweb/packages/agenta-observability/src/state/selectors.tsweb/packages/agenta-observability/src/utils/buildTraceQueryParams.tsweb/packages/agenta-observability/src/utils/filterCoercion.tsweb/packages/agenta-observability/tests/unit/queryHelpers.test.tsweb/packages/agenta-observability/tsconfig.jsonweb/packages/agenta-observability/vitest.config.ts
💤 Files with no reviewable changes (1)
- web/oss/src/state/newObservability/index.ts
🛑 Comments failed to post (1)
web/packages/agenta-observability/src/etl/withRateLimitRetry.ts (1)
53-59: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Honor a zero-second
Retry-Aftervalue.
Retry-After: 0means retry immediately. The current> 0checks use the 10-second fallback instead. Accept zero for both header and message values.Proposed fix
- if (Number.isFinite(headerSeconds) && headerSeconds > 0) { + if (Number.isFinite(headerSeconds) && headerSeconds >= 0) { delayMs = headerSeconds * 1000 ... - if (Number.isFinite(seconds) && seconds > 0) delayMs = seconds * 1000 + if (Number.isFinite(seconds) && seconds >= 0) delayMs = seconds * 1000📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (Number.isFinite(headerSeconds) && headerSeconds >= 0) { delayMs = headerSeconds * 1000 } else { const match = (e?.message ?? "").match(/retry after (\d+)\s*second/i) if (match) { const seconds = Number.parseInt(match[1], 10) if (Number.isFinite(seconds) && seconds >= 0) delayMs = seconds * 1000
1 participant
Context
The observability page's state lived in
oss/src/state/newObservability, which meant mobile could not read a trace list without importing from the desktop app. This moves that state layer into@agenta/observability.Second of five stacked PRs. Base is
obs/wp0-entities-prep.Changes
The atoms, query helpers, constants and trace-span types move into
@agenta/observability.oss/src/state/observabilitystays, but shrinks to the app's binding: the atoms it re-binds, aseams.tsfor the app-only wiring, anduseObservability.newObservabilityis gone rather than left as a shim. OSS and EE lint-block re-exporting@agenta/*, so the call sites are rewritten instead.SortResultbecomes an alias ofAnalyticsRange. They were already the same shape, and naming that here is what later lets one range picker serve both the dashboard and the traces toolbar.Tests / notes
@agenta/observabilitybuilds, lints and passes its unit tests.@typescript-eslint/no-explicit-anyto error, so everyanythat came across had to be resolved rather than carried.