perf: short-circuit observe() when OTEL_ENABLE_EXTRA_METRICS is disabled#1087
perf: short-circuit observe() when OTEL_ENABLE_EXTRA_METRICS is disabled#1087hugo-ccabral wants to merge 1 commit intomainfrom
Conversation
When extra metrics are disabled (the common production case), observe() now directly calls f() without any wrapping. Previously it always wrapped in an async function with performance.now(), try/catch, and isWrappedError check, even when metrics were off. CPU profile showed observability overhead at ~0.9 percent with 548 hits as self time. Co-authored-by: Cursor <cursoragent@cursor.com>
Tagging OptionsShould a new tag be published when this PR is merged?
|
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@observability/observe.ts`:
- Around line 5-10: The observeWithMetrics IIFE eagerly calls
meter.createHistogram (operationDuration) at import time; change
observeWithMetrics into a lazily-initialized function that checks
OTEL_ENABLE_EXTRA_METRICS (or the existing enable flag) on first invocation,
creates operationDuration via meter.createHistogram only when enabled, caches it
for subsequent calls, and returns a no-op behavior when disabled so no histogram
is created during import; update any references to observeWithMetrics to use the
new function shape if necessary.
- Around line 40-45: The disabled branch of the exported observe function can
throw synchronously because it directly invokes f(), violating the Promise
contract; change the fallback implementation to wrap the invocation in a
resolved Promise (e.g., use Promise.resolve().then(() => f()) or
Promise.resolve().then(f)) so any synchronous throw becomes a rejected Promise;
update the branch that currently returns (_op, f) => f() to instead return (_op,
f) => Promise.resolve().then(() => f()) while keeping the same signature and
preserving use of observeWithMetrics when OTEL_ENABLE_EXTRA_METRICS is true.
| const observeWithMetrics = (() => { | ||
| const operationDuration = meter.createHistogram("block_op_duration", { | ||
| description: "operation duration", | ||
| unit: "ms", | ||
| valueType: ValueType.DOUBLE, | ||
| }); |
There was a problem hiding this comment.
observeWithMetrics is still eagerly initialized when metrics are disabled.
Line 5 uses an IIFE, so meter.createHistogram(...) on Line 6 runs at import time even when OTEL_ENABLE_EXTRA_METRICS is false. That defeats the lazy-init/perf goal in disabled mode.
💡 Proposed fix
-const observeWithMetrics = (() => {
+const createObserveWithMetrics = () => {
const operationDuration = meter.createHistogram("block_op_duration", {
description: "operation duration",
unit: "ms",
valueType: ValueType.DOUBLE,
});
return async <T>(
op: string,
f: () => Promise<T>,
): Promise<T> => {
@@
- };
-})();
+ };
+};
export const observe: <T>(
op: string,
f: () => Promise<T>,
) => Promise<T> = OTEL_ENABLE_EXTRA_METRICS
- ? observeWithMetrics
+ ? createObserveWithMetrics()
: (_op, f) => f();Also applies to: 34-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@observability/observe.ts` around lines 5 - 10, The observeWithMetrics IIFE
eagerly calls meter.createHistogram (operationDuration) at import time; change
observeWithMetrics into a lazily-initialized function that checks
OTEL_ENABLE_EXTRA_METRICS (or the existing enable flag) on first invocation,
creates operationDuration via meter.createHistogram only when enabled, caches it
for subsequent calls, and returns a no-op behavior when disabled so no histogram
is created during import; update any references to observeWithMetrics to use the
new function shape if necessary.
| export const observe: <T>( | ||
| op: string, | ||
| f: () => Promise<T>, | ||
| ) => Promise<T> = OTEL_ENABLE_EXTRA_METRICS | ||
| ? observeWithMetrics | ||
| : (_op, f) => f(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the observability directory and the observe.ts file
fd -t f observe.tsRepository: deco-cx/deco
Length of output: 80
🏁 Script executed:
# Read the observe.ts file to see full context
cat -n observability/observe.tsRepository: deco-cx/deco
Length of output: 1555
🏁 Script executed:
# Search for callers of the observe function
rg "observe\(" -A 3 -B 1 --type ts --type jsRepository: deco-cx/deco
Length of output: 1210
🏁 Script executed:
# Check git history to see previous implementations
git log --oneline -20 -- observability/observe.tsRepository: deco-cx/deco
Length of output: 136
🏁 Script executed:
# Look for tests of the observe function
fd -t f "*.test\|*.spec" | xargs grep -l "observe"Repository: deco-cx/deco
Length of output: 351
🏁 Script executed:
# Search for imports of observe function from observe.ts
rg "from ['\"].*observe" --type ts --type js | head -20Repository: deco-cx/deco
Length of output: 125
🏁 Script executed:
# Search for "observe" being called with a function argument
rg "observe\s*\(" -A 2 --type ts --type js | grep -B 2 -A 2 "observe.*=>|observe.*function"Repository: deco-cx/deco
Length of output: 38
🏁 Script executed:
# Get the git diff for the perf commit
git show f5997fa --statRepository: deco-cx/deco
Length of output: 18468
🏁 Script executed:
# Check what the previous implementation looked like
git show f5997faRepository: deco-cx/deco
Length of output: 50372
🏁 Script executed:
# Find actual calls to observe() from observe.ts module
rg "observe\(" runtime/mod.ts -A 3 -B 1Repository: deco-cx/deco
Length of output: 38
🏁 Script executed:
# Search for how observe is used across the codebase
rg "observe\(" --type ts -B 2 -A 5 | grep -A 7 -B 3 "observe.*Promise\|observe.*async\|observe.*catch"Repository: deco-cx/deco
Length of output: 38
🏁 Script executed:
# Look for error handling around observe calls
rg "observe\(" --type ts --type js -A 10 | head -50Repository: deco-cx/deco
Length of output: 1888
🏁 Script executed:
# Read runtime/mod.ts to see how observe is actually used
cat -n runtime/mod.ts | head -100Repository: deco-cx/deco
Length of output: 4211
🏁 Script executed:
# Search more carefully for the observe function usage pattern
rg "observe\s*\(" runtime/mod.ts -A 5 -B 2Repository: deco-cx/deco
Length of output: 38
🏁 Script executed:
# Look for the exact pattern where observe() is called with a function
rg "observe\(['\"]" --type ts --type js -A 3Repository: deco-cx/deco
Length of output: 38
🏁 Script executed:
# Check if there are any tests for the observe function
find . -name "*.test.ts" -o -name "*.spec.ts" | xargs grep -l "observe" 2>/dev/null || echo "No test files found matching observe"Repository: deco-cx/deco
Length of output: 92
🏁 Script executed:
# Search for actual invocations of observe() with string and function
rg "observe\s*\(\s*['\"]" --type ts --type js -A 3Repository: deco-cx/deco
Length of output: 38
🏁 Script executed:
# Look at more of runtime/mod.ts to find where observe is called
cat -n runtime/mod.ts | sed -n '100,276p'Repository: deco-cx/deco
Length of output: 5771
🏁 Script executed:
# Search for "observe(" as a function call in the entire codebase
rg "observe\s*\(\s*['\"]" --type ts --type jsRepository: deco-cx/deco
Length of output: 38
🏁 Script executed:
# Check if observe function is used in any render or invoke functions
rg "observe" runtime/features/render.tsx -A 3 -B 2
rg "observe" runtime/features/invoke.ts -A 3 -B 2
rg "observe" runtime/features/meta.ts -A 3 -B 2Repository: deco-cx/deco
Length of output: 38
🏁 Script executed:
# Search for how monitoring.metrics is used
rg "monitoring\.metrics\|state\.monitoring\.metrics" --type ts --type js -A 3 -B 1Repository: deco-cx/deco
Length of output: 38
🏁 Script executed:
# Search for uses of the metrics property in monitoring
rg "\.metrics\s*\(" --type ts --type js -A 3 -B 1 | head -50Repository: deco-cx/deco
Length of output: 38
🏁 Script executed:
# Look for where observe might be called as a function
rg "metrics\s*\(\s*['\"]" --type ts --type js -A 2Repository: deco-cx/deco
Length of output: 38
🏁 Script executed:
# Look for where the function passed to observe might throw
rg "observe\|metrics" engine/ --type ts -A 5 | head -80Repository: deco-cx/deco
Length of output: 38
🏁 Script executed:
# Check the resolver to see how observe/metrics is used
rg "monitoring" engine/core/resolver.ts -A 3 -B 3Repository: deco-cx/deco
Length of output: 1006
🏁 Script executed:
# Look at blocks/loader.ts since observe is imported with isWrappedError from there
rg "isWrappedError" blocks/loader.ts -A 3 -B 3Repository: deco-cx/deco
Length of output: 234
🏁 Script executed:
# Search for any synchronous error throwing patterns in the codebase
rg "throw\s+new\s+Error|throw\s+\w" blocks/loader.ts -B 2 -A 2 | head -40Repository: deco-cx/deco
Length of output: 327
🏁 Script executed:
# Check if there are any function blocks that might synchronously throw before returning Promise
rg "observe\(" --type ts -B 5 -A 5 runtime/ | head -100Repository: deco-cx/deco
Length of output: 856
🏁 Script executed:
# Check if metricsFunc is actually used in resolver.ts
cat -n engine/core/resolver.ts | grep -A 10 -B 5 "metricsFunc"Repository: deco-cx/deco
Length of output: 1206
🏁 Script executed:
# Search for any patterns where observe might be invoked
rg "observe\s*\(\s*[\"\']|metrics\s*\(\s*[\"\']" --type ts --type jsRepository: deco-cx/deco
Length of output: 180
🏁 Script executed:
# Look at the complete monitoring type definition
rg "interface Monitoring|type Monitoring" --type ts -A 10Repository: deco-cx/deco
Length of output: 566
🏁 Script executed:
# Check what the previous version of observe looked like before the perf commit
git show f5997fa^:observability/observe.ts 2>/dev/null || echo "Cannot retrieve previous version"Repository: deco-cx/deco
Length of output: 88
🏁 Script executed:
# Find where metricsFunc is actually called with arguments
rg "metricsFunc" engine/core/resolver.ts -A 5 -B 2Repository: deco-cx/deco
Length of output: 796
🏁 Script executed:
# Check the ObserveFunc type definition
rg "type ObserveFunc|interface ObserveFunc" --type ts -A 3Repository: deco-cx/deco
Length of output: 240
🏁 Script executed:
# Verify that the function passed to observe at line 478 is always async
sed -n '476,490p' engine/core/resolver.tsRepository: deco-cx/deco
Length of output: 786
🏁 Script executed:
# Check if there are any other callers of metrics function besides the one in resolver
rg "metrics\?" engine/core/resolver.ts -n | grep -v "monitoring"Repository: deco-cx/deco
Length of output: 38
🏁 Script executed:
# Check if there are any other uses of observe/metrics in the codebase
rg "metricsFunc" engine/ --type ts -A 3 -B 1Repository: deco-cx/deco
Length of output: 784
🏁 Script executed:
# Verify the complete call signature at line 478
sed -n '475,495p' engine/core/resolver.tsRepository: deco-cx/deco
Length of output: 926
🏁 Script executed:
# Check if the function signature constraint is enforced
rg "ObserveFunc|<T>\(" observability/observe.ts -B 2 -A 3Repository: deco-cx/deco
Length of output: 323
🏁 Script executed:
# Do a final check - see if any tests would break with the proposed fix
find . -name "*.test.ts" -o -name "*.spec.ts" | head -20Repository: deco-cx/deco
Length of output: 254
🏁 Script executed:
# Check if there are any other places where observe might be called besides the one in resolver.ts
rg "observe\(" --type ts --type js | grep -v "observer\|IntersectionObserver"Repository: deco-cx/deco
Length of output: 521
Disabled branch can break Promise contract via synchronous throws.
At line 45, (_op, f) => f() may throw synchronously if f throws before returning a Promise. The type signature requires f: () => Promise<T>, which provides compile-time protection. In practice, the sole caller in engine/core/resolver.ts always passes an async function that cannot throw synchronously. However, the type constraint can be violated at runtime.
For stricter Promise contract compliance, consider wrapping the call:
Proposed fix
) => Promise<T> = OTEL_ENABLE_EXTRA_METRICS
? observeWithMetrics
- : (_op, f) => f();
+ : (_op, f) => Promise.resolve().then(f);📝 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.
| export const observe: <T>( | |
| op: string, | |
| f: () => Promise<T>, | |
| ) => Promise<T> = OTEL_ENABLE_EXTRA_METRICS | |
| ? observeWithMetrics | |
| : (_op, f) => f(); | |
| export const observe: <T>( | |
| op: string, | |
| f: () => Promise<T>, | |
| ) => Promise<T> = OTEL_ENABLE_EXTRA_METRICS | |
| ? observeWithMetrics | |
| : (_op, f) => Promise.resolve().then(f); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@observability/observe.ts` around lines 40 - 45, The disabled branch of the
exported observe function can throw synchronously because it directly invokes
f(), violating the Promise contract; change the fallback implementation to wrap
the invocation in a resolved Promise (e.g., use Promise.resolve().then(() =>
f()) or Promise.resolve().then(f)) so any synchronous throw becomes a rejected
Promise; update the branch that currently returns (_op, f) => f() to instead
return (_op, f) => Promise.resolve().then(() => f()) while keeping the same
signature and preserving use of observeWithMetrics when
OTEL_ENABLE_EXTRA_METRICS is true.
1 participant
Summary
OTEL_ENABLE_EXTRA_METRICSis disabled (common production case),observe()now directly callsf()without any wrappingperformance.now(),try/catch, andisWrappedError()checkContext
CPU profile showed observability overhead at ~0.9% of total CPU (548 hits as self time in
observe.ts+timings.ts). This is called for every resolver invocation in the pipeline.Test plan
OTEL_ENABLE_EXTRA_METRICSis setSummary by cubic
Short-circuits observe() when OTEL_ENABLE_EXTRA_METRICS is disabled, calling the resolver directly. This removes per-call wrapper overhead and reduces CPU (~0.9% in profiles).
Written for commit f5997fa. Summary will update on new commits.
Summary by CodeRabbit
Release Notes