Skip to content

perf: short-circuit observe() when OTEL_ENABLE_EXTRA_METRICS is disabled #1087

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
hugo-ccabral wants to merge 1 commit into
base: main
Choose a base branch
from
+34 −26
Open

perf: short-circuit observe() when OTEL_ENABLE_EXTRA_METRICS is disabled #1087

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
60 changes: 34 additions & 26 deletions observability/observe.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,36 +2,44 @@ import { isWrappedError } from "../blocks/loader.ts";
import { ValueType } from "../deps.ts";
import { meter, OTEL_ENABLE_EXTRA_METRICS } from "./otel/metrics.ts";

const operationDuration = meter.createHistogram("block_op_duration", {
description: "operation duration",
unit: "ms",
valueType: ValueType.DOUBLE,
});
const observeWithMetrics = (() => {
const operationDuration = meter.createHistogram("block_op_duration", {
description: "operation duration",
unit: "ms",
valueType: ValueType.DOUBLE,
});
Comment on lines +5 to +10
Copy link

@coderabbitai coderabbitai bot Feb 25, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.


/**
* Observe function durations based on the provided labels
*/
export const observe = async <T>(
op: string,
f: () => Promise<T>,
): Promise<T> => {
const start = performance.now();
let isError = "false";
try {
const result = await f();
if (isWrappedError(result)) {
return async <T>(
op: string,
f: () => Promise<T>,
): Promise<T> => {
const start = performance.now();
let isError = "false";
try {
const result = await f();
if (isWrappedError(result)) {
isError = "true";
}
return result;
} catch (error) {
isError = "true";
}
return result;
} catch (error) {
isError = "true";
throw error;
} finally {
if (OTEL_ENABLE_EXTRA_METRICS) {
throw error;
} finally {
operationDuration.record(performance.now() - start, {
"operation.name": op,
"operation.is_error": isError,
});
}
}
};
};
})();

/**
* When metrics are disabled, pass through directly to avoid async wrapper
* overhead (performance.now, try/catch, isWrappedError check).
*/
export const observe: <T>(
op: string,
f: () => Promise<T>,
) => Promise<T> = OTEL_ENABLE_EXTRA_METRICS
? observeWithMetrics
: (_op, f) => f();
Comment on lines +40 to +45
Copy link

@coderabbitai coderabbitai bot Feb 25, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, locate the observability directory and the observe.ts file
fd -t f observe.ts

Repository: deco-cx/deco

Length of output: 80

🏁 Script executed:

# Read the observe.ts file to see full context
cat -n observability/observe.ts

Repository: 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 js

Repository: deco-cx/deco

Length of output: 1210

🏁 Script executed:

# Check git history to see previous implementations
git log --oneline -20 -- observability/observe.ts

Repository: 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 -20

Repository: 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 --stat

Repository: deco-cx/deco

Length of output: 18468

🏁 Script executed:

# Check what the previous implementation looked like
git show f5997fa

Repository: 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 1

Repository: 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 -50

Repository: 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 -100

Repository: 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 2

Repository: 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 3

Repository: 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 3

Repository: 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 js

Repository: 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 2

Repository: 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 1

Repository: 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 -50

Repository: 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 2

Repository: 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 -80

Repository: 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 3

Repository: 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 3

Repository: 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 -40

Repository: 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 -100

Repository: 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 js

Repository: deco-cx/deco

Length of output: 180

🏁 Script executed:

# Look at the complete monitoring type definition
rg "interface Monitoring|type Monitoring" --type ts -A 10

Repository: 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 2

Repository: deco-cx/deco

Length of output: 796

🏁 Script executed:

# Check the ObserveFunc type definition
rg "type ObserveFunc|interface ObserveFunc" --type ts -A 3

Repository: 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.ts

Repository: 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 1

Repository: deco-cx/deco

Length of output: 784

🏁 Script executed:

# Verify the complete call signature at line 478
sed -n '475,495p' engine/core/resolver.ts

Repository: 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 3

Repository: 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 -20

Repository: 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.

Suggested change
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.

Loading