feat(observability): align metrics with OTel semantic conventions - #1211
Open
nicacioliveira wants to merge 5 commits into
Open
feat(observability): align metrics with OTel semantic conventions#1211nicacioliveira wants to merge 5 commits into
nicacioliveira wants to merge 5 commits into
Conversation
Use official @opentelemetry/semantic-conventions constants (bumped 1.25.1 -> 1.37.0) instead of hardcoded strings, and rename/retype metrics to semconv: - http_request_duration (ms) -> http.server.request.duration (seconds), with semconv attributes http.request.method / http.route / http.response.status_code (status as int, not string). - block_op_duration (ms) -> deco.block.operation.duration (seconds). - cache_hit -> deco.cache.hits; cache attributes namespaced under deco.cache.*. - Resource attributes migrated off the deprecated SemanticResourceAttributes to ATTR_* constants; deployment.environment -> deployment.environment.name. - Seconds histogram buckets widened to cover sub-second HTTP latencies through multi-second operations. deco-proprietary names (no semconv equivalent) live in observability/otel/ conventions.ts. Clean rename — no dual-emit (ClickStack is a fresh backend). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Tagging OptionsShould a new tag be published when this PR is merged?
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR replaces hard-coded OpenTelemetry metric and attribute names with shared convention constants, updates resource attributes to the new semconv exports, records duration metrics in seconds, adjusts histogram buckets, and updates cache instrumentation keys. ChangesOpenTelemetry telemetry updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
observability/http.ts (1)
19-25: 🗄️ Data Integrity & Integration | 🔵 TrivialClarify call site behavior regarding
ATTR_HTTP_ROUTEThe concern about unbounded cardinality is resolved. The return function's
pathparameter is correctly called withctx?.var?.pathTemplateinruntime/middleware.ts, ensuring the metric receives the low-cardinality route template (e.g.,/users/:id) rather than a raw URL path.The parameter name
pathin the closure withinobservability/http.tsis slightly misleading. Renaming it torouteorrouteTemplatewould improve code clarity.🤖 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 `@observability/http.ts` around lines 19 - 25, The `startObserve` closure in `observability/http.ts` already receives the low-cardinality route template from `runtime/middleware.ts`, but the local parameter name `path` is misleading. Rename that parameter to `route` or `routeTemplate` and update the `httpDuration.record` attributes usage accordingly so the intent is clear without changing behavior.
🤖 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 `@observability/otel/config.ts`:
- Around line 43-44: The OpenTelemetry resource mapping is using
instance-specific values for service.version, which should instead be a stable
build/release identifier. Update the attributes in the config setup so
ATTR_SERVICE_VERSION is sourced from a fixed version string (for example a
package/build version or git hash), and keep ATTR_SERVICE_INSTANCE_ID as the
unique runtime instance value. Use the existing resource attribute assignment in
config.ts where ATTR_SERVICE_VERSION and ATTR_SERVICE_INSTANCE_ID are set to
make the change.
In `@runtime/caches/common.ts`:
- Around line 52-57: The cache hit counter in the common cache flow is being
incremented for every lookup result, including misses, which makes
`deco.cache.hits` misleading. Update the logic in `getCacheStatus`/the
`cacheHit.add` call site so only true hits increment this instrument, or change
the metric name/semantics to a neutral lookup counter if misses should remain
included; keep the existing `ATTR_DECO_CACHE_STATUS`, `ATTR_DECO_CACHE_RESULT`,
and `ATTR_DECO_CACHE_ENGINE` labels aligned with the intended metric contract.
---
Nitpick comments:
In `@observability/http.ts`:
- Around line 19-25: The `startObserve` closure in `observability/http.ts`
already receives the low-cardinality route template from
`runtime/middleware.ts`, but the local parameter name `path` is misleading.
Rename that parameter to `route` or `routeTemplate` and update the
`httpDuration.record` attributes usage accordingly so the intent is clear
without changing behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f9e45cc4-7e87-4641-b071-5b4592638693
📒 Files selected for processing (7)
deps.tsobservability/http.tsobservability/observe.tsobservability/otel/config.tsobservability/otel/conventions.tsobservability/otel/metrics.tsruntime/caches/common.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="deps.ts">
<violation number="1" location="deps.ts:94">
P2: Re-exporting from `/incubating` entry point violates OTel's explicit recommendation. Copy the 5 needed experimental definitions into `observability/otel/conventions.ts` (already exists per PR desc) instead of binding consumers to an unstable API.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The counter records every cache lookup dimensioned by deco.cache.result (hit/stale/miss), so "hits" was a misnomer and collided with @decocms/start's hits/misses counters. Rename to deco.cache.lookups — unified cache metric across both frameworks (single counter + deco.cache.result dimension). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tart) Use the metric names @decocms/start already established (two counters) instead of inventing deco.cache.lookups. Both dimensioned by deco.cache.result. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@runtime/caches/common.ts`:
- Around line 18-24: The CacheMetrics shape is out of sync with the counter
implementation: it still exposes total and hits even though common.ts now
creates separate cacheHits and cacheMisses counters. Update the exported
CacheMetrics interface to match the new metrics structure, using the existing
cacheHits/cacheMisses symbols from the module, or remove the interface if
nothing consumes it anymore.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 766ba6b7-7d00-4c99-9dd6-8ee4e8eea563
📒 Files selected for processing (2)
observability/otel/conventions.tsruntime/caches/common.ts
✅ Files skipped from review due to trivial changes (1)
- observability/otel/conventions.ts
Comment on lines
+18
to
+24
| // Two counters (names match @decocms/start); `deco.cache.result` carries the | ||
| // outcome (hit/stale/miss) for both. | ||
| const cacheHits = meter.createCounter(METRIC_DECO_CACHE_HITS, { | ||
| unit: "1", | ||
| valueType: ValueType.DOUBLE, | ||
| }); | ||
| const cacheMisses = meter.createCounter(METRIC_DECO_CACHE_MISSES, { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== runtime/caches/common.ts (top of file) =="
sed -n '1,50p' runtime/caches/common.ts
echo
echo "== CacheMetrics references =="
rg -n -C2 '\bCacheMetrics\b' runtime observabilityRepository: deco-cx/deco
Length of output: 1882
Update CacheMetrics interface to reflect separate hit/miss counters
The exported CacheMetrics interface still defines total and hits fields, but the internal implementation now creates distinct cacheHits and cacheMisses counters. Synchronize the interface definition to match this new structure or verify if the interface is unused and can be removed.
🤖 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 `@runtime/caches/common.ts` around lines 18 - 24, The CacheMetrics shape is out
of sync with the counter implementation: it still exposes total and hits even
though common.ts now creates separate cacheHits and cacheMisses counters. Update
the exported CacheMetrics interface to match the new metrics structure, using
the existing cacheHits/cacheMisses symbols from the module, or remove the
interface if nothing consumes it anymore.
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="runtime/caches/common.ts">
<violation number="1" location="runtime/caches/common.ts:24">
P3: The exported `CacheMetrics` interface still declares `total` and `hits` but the implementation now tracks hits and misses via two separate counters (`cacheHits` / `cacheMisses`). Either update the interface to reflect the new structure (e.g., add a `misses` field) or remove it if it's no longer consumed.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| unit: "1", | ||
| valueType: ValueType.DOUBLE, | ||
| }); | ||
| const cacheMisses = meter.createCounter(METRIC_DECO_CACHE_MISSES, { |
Contributor
There was a problem hiding this comment.
P3: The exported CacheMetrics interface still declares total and hits but the implementation now tracks hits and misses via two separate counters (cacheHits / cacheMisses). Either update the interface to reflect the new structure (e.g., add a misses field) or remove it if it's no longer consumed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/caches/common.ts, line 24:
<comment>The exported `CacheMetrics` interface still declares `total` and `hits` but the implementation now tracks hits and misses via two separate counters (`cacheHits` / `cacheMisses`). Either update the interface to reflect the new structure (e.g., add a `misses` field) or remove it if it's no longer consumed.</comment>
<file context>
@@ -14,7 +15,13 @@ export interface CacheMetrics {
+ unit: "1",
+ valueType: ValueType.DOUBLE,
+});
+const cacheMisses = meter.createCounter(METRIC_DECO_CACHE_MISSES, {
unit: "1",
valueType: ValueType.DOUBLE,
</file context>
…status Follow OTel semconv modeling (no canonical cache metric exists; mirror the nfs.server.repcache.requests + .status pattern + the prefer-attributes-over- metrics guidance). One counter dimensioned by deco.cache.status (hit/stale/ miss). Same key on span + metric. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
igoramf
approved these changes
Jun 26, 2026
hugo-ccabral
approved these changes
Jun 26, 2026
…e.version - Stop re-exporting from @opentelemetry/semantic-conventions/incubating (OTel advises against depending on the unstable entry point); vendor the 4 needed experimental attribute names as plain constants in conventions.ts. - service.version: use the deployment revision (deploymentId) falling back to the framework version, not Deno.hostname() (which is instance identity). Addresses PR review (cubic P2 + coderabbit). Cache is already a single deco.cache.requests counter + deco.cache.status label (earlier commit). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Review addressed:
|
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
3 participants
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.
What
Bring the framework's telemetry in line with OpenTelemetry semantic conventions, using the official
@opentelemetry/semantic-conventionsconstants (no hardcoded name strings).Metrics renamed + retyped
http_request_duration(ms)http.server.request.duration(seconds)http.method/http.response.status(string)http.request.method/http.response.status_code(int)block_op_duration(ms)deco.block.operation.duration(seconds)cache_hitdeco.cache.hits(attrs underdeco.cache.*)Resource attributes
SemanticResourceAttributestoATTR_*constants (@opentelemetry/semantic-conventions1.25.1 → 1.37.0, stable +/incubating).deployment.environment→deployment.environment.name.Notes
http.request.method,http.response.status_code,http.route,url.*,server.address) — unchanged.observability/otel/conventions.ts.deno checkpasses on all touched files.Part of the HyperDX → ClickStack migration (normalize at source: semconv + seconds). Companion PR aligns
deco-cx/appsai-assistants togen_ai.*.🤖 Generated with Claude Code
Summary by cubic
Aligns our metrics and resource attributes with OpenTelemetry semantic conventions, switches duration units to seconds using
@opentelemetry/semantic-conventionsconstants, and corrects service.version to use the deployment revision. Standardizes HTTP, block operation, and cache telemetry for ClickStack; no dual-emit.Refactors
Dependencies
@opentelemetry/semantic-conventionsto 1.37.0 and stopped importing from/incubating; vendored the few incubating attribute names in observability/otel/conventions.ts and replaced deprecated SemanticResourceAttributes usage.Written for commit 8f9a960. Summary will update on new commits.
Summary by CodeRabbit