feat(aws): aws-durable-execution-sdk-js instrumentation#8012
feat(aws): aws-durable-execution-sdk-js instrumentation#8012pablomartinezbernardo merged 64 commits into
Conversation
joeyzhao2018
changed the title
Overall package sizeSelf size: 6.24 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.0.2 | 85.93 kB | 825.11 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
🎉 All green!🧪 All tests passed 🔗 Commit SHA: bc54581 | Docs | Datadog PR Page | Give us feedback! |
BenchmarksBenchmark execution time: 2026-06-16 12:15:24 Comparing candidate commit bc54581 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1936 metrics, 14 unstable metrics.
|
wconti27
added
the
apm-integration-toolkit
c1c351e to
7756bba
Compare
| function getOperationId (ctxImpl) { | ||
| const stepId = ctxImpl?.getNextStepId?.() | ||
| if (!stepId) return | ||
| return createHash('md5').update(stepId).digest('hex').slice(0, 16) | ||
| } |
There was a problem hiding this comment.
Is replicating this acceptable? It's defined here, but doesn't seem like we can access it https://github.com/aws/aws-durable-execution-sdk-js/blob/main/packages/aws-durable-execution-sdk-js/src/utils/step-id-utils/step-id-utils.ts
There was a problem hiding this comment.
Should be fine I would think
There was a problem hiding this comment.
It's not ideal due to the CPU overhead.
Are we able to modify the source code of the file generating the hash to be able to access it? E.g. using orchestrion?
There was a problem hiding this comment.
Is the CPU overhead the issue? Even if we were able to access it using orchestrion (which doesn't seem to be the case for being private), and we could call it, we would just be running the same code.
This hash is used for 2 things by aws
- Accessing replayed operations, the unique hash is used to have direct access in a dict
- Showing it in the UI
There is no place where the hash is calculated for fresh (meaning non-replayed) operations, there's no place where we can hook onto to get this value for fresh operations.
The drawback I see from this is actually having to replicate code in our tracer, but I can't see a way to avoid it if we need to have the operation hash.
There was a problem hiding this comment.
With Orchestrion-JS we should be able to replace existing code of the library. Thus, we would be able to intercept the original implementation without additional overhead.
About CPU overhead: we are currently working hard to get the tracer to perform super fast.
There was a problem hiding this comment.
The issue is that there is no good place to intercept this function call, it's not reliably called at any point in the process where we can correlate it with the span.
If this is unacceptable, the alternative may actually be removing this field. This will impact other components though
pablomartinezbernardo
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a893b2db18
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const data = ctx?.arguments?.[1] | ||
| if (data?.Action !== 'RETRY' || !data.Error) return | ||
|
|
||
| const span = this.activeSpan |
There was a problem hiding this comment.
| const span = this.activeSpan | |
| const span = ctx.currentStore?.span |
There was a problem hiding this comment.
AwsDurableExecutionSdkJsCheckpointPlugin doesn't create spans, it just enriches the span. As it doesn't create a span, ctx.currentStore.span will be undefined
There was a problem hiding this comment.
Are we certain the activeSpan is the correct checkpoint part? Could the checkpoint run outside of original context? We would place the checkpoint on the wrong span in that case.
There was a problem hiding this comment.
We are certain, checkpointing is done synchronously here
| if (!pkgJson) { | ||
| const requireFromWorkspace = createRequire(join(folder, 'package.json')) | ||
| const nodeModulesPaths = requireFromWorkspace.resolve.paths(externalName) | ||
| pkgJson = requirePackageJson(externalName, { paths: nodeModulesPaths }) |
There was a problem hiding this comment.
This should be handled under the hood already and is unnecessary.
There was a problem hiding this comment.
Oh yeah forgot to mention this
aws-durable-execution-sdk-js only exports .
pkgJsonPath does require.resolve('@aws/durable-execution-sdk-js/package.json') which throws ERR_PACKAGE_PATH_NOT_EXPORTED.
From what I've seen the way to solve properly would be this https://nodejs.org/api/module.html#modulefindpackagejsonspecifier-base but... node 22
There was a problem hiding this comment.
A lot of libraries do this, why are we only hitting this issue now?
There was a problem hiding this comment.
Because this doesn't happen that often: assertPeerDependencies is only run for libraries with dep: true in externals.js. And from all those, this is the only case where exports is used without exporting the package.json, but could happen for other libraries as well
There was a problem hiding this comment.
I have a feeling this won't work properly when the customer bundles their application. Can you throw an acceptance test into the esbuild integration tests directory that installs this package? You should see other examples in there when we discover a tricky package.
There was a problem hiding this comment.
Added the acceptance test e1a2f7c . The exports restriction shouldn't affect bundling, no? Both the esbuild plugin and the runtime resolver read package.json off disk, this fix is only for CI workspace generation.
| if (!span || span._spanContext?._tags?.error) return | ||
|
|
||
| const { ErrorMessage, ErrorType, StackTrace } = data.Error | ||
| span.setTag('error', 1) |
There was a problem hiding this comment.
Why are we not creating a span? Otherwise the only options are overwriting an existing error or dropping it completely, both of which are not ideal.
There was a problem hiding this comment.
Why are we not creating a span?
Because checkpointing is not an operation we really want to trace for the customer, the user doesn't run the checkpoint operation: the SDK checkpoints automatically after each step execution.
The error is first visible at the beginning of the checkpoint. Steps don't throw themselves when there is a user error (They could throw if there's a bug in the SDK when creating the promise, though). This condition shouldn't ever happen, no error should be set on the span prior to us seeing the checkpoint.
Do you think removing the condition would make it clearer?
There was a problem hiding this comment.
We trace what the process is doing, not what the user is doing, so if checkpoints are happening shouldn't we be tracing them? Especially if it's an operation that can fail. Setting the error on another span is way more confusing.
There was a problem hiding this comment.
The data.Error here isn't a checkpoint failure, it's the user step's error object. On Action: RETRY the SDK doesn't throw or settle the step's promise, so the step's settle channel never fires. The checkpoint is where we can observe the user error. This is also the reason why we are finishing the span here.
Checkpoint operations themselves can fail only because of network/AWS errors, but they are already traced as a generic aws.request, they don't contain additional information relevant to durable functions other than the error on the step we are retrieving from here.
| if (ErrorType) span.setTag('error.type', ErrorType) | ||
| if (Array.isArray(StackTrace)) span.setTag('error.stack', StackTrace.join('\n')) | ||
|
|
||
| ctx._ddRetryStepSpan = span |
There was a problem hiding this comment.
Why use an underscore property on the context?
There was a problem hiding this comment.
To be able to finish the span in the asyncEnd. We could also either
a) finish it in start, the difference in time wouldn't be that big, but it would finish earlier
b) actually do this.activeSpan?.finish() on asyncEnd, but we would need to check again for conditions like data?.Action !== 'RETRY'
Would you rather see any of these? Or maybe I'm misunderstanding your question?
There was a problem hiding this comment.
What I meant was why is the property underscored?
There was a problem hiding this comment.
Oh, because claude (seemingly incorrectly) chose it, and it looked reasonable. Seems like this PR is the first and only one that introduced this pattern, maybe we want to update it as well so AIs don't get confused? #7669
In any case, updated the name in my PR.
| * rejection reason on failure. | ||
| * @returns {void} | ||
| */ | ||
| function observeDurablePromise (dp, onSettle) { |
There was a problem hiding this comment.
We should not be patching on the plugin side. Any way to instead patch the thenable directly at the source with a corresponding additional channel?
There was a problem hiding this comment.
Thanks, makes sense, addressed in 5289e4d can you have a look?
| if (!pkgJson) { | ||
| const requireFromWorkspace = createRequire(join(folder, 'package.json')) | ||
| const nodeModulesPaths = requireFromWorkspace.resolve.paths(externalName) | ||
| pkgJson = requirePackageJson(externalName, { paths: nodeModulesPaths }) |
There was a problem hiding this comment.
A lot of libraries do this, why are we only hitting this issue now?
…/aws-durable-execution-sdk-js # Conflicts: # supported_versions_output.json # supported_versions_table.csv
…/aws-durable-execution-sdk-js # Conflicts: # packages/datadog-instrumentations/src/helpers/rewriter/instrumentations/index.js # packages/dd-trace/test/plugins/versions/package.json # supported_versions_table.csv
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
Besides existing comments, this looks pretty solid! I did not review the tests closely though. I might have missed that but I believe we could also add an ESM test?
…er.js Co-authored-by: Ruben Bridgewater <[email protected]>
| while (typeof err.errorType === 'string' && err.cause instanceof Error) { | ||
| err = err.cause | ||
| } | ||
| return { ...ctxOrError, error: err } |
There was a problem hiding this comment.
| return { ...ctxOrError, error: err } | |
| return err |
I think we only need the error :)
| const data = ctx?.arguments?.[1] | ||
| if (data?.Action !== 'RETRY' || !data.Error) return | ||
|
|
||
| const span = this.activeSpan |
There was a problem hiding this comment.
Are we certain the activeSpan is the correct checkpoint part? Could the checkpoint run outside of original context? We would place the checkpoint on the wrong span in that case.
|
|
||
| module.exports = { | ||
| // Only list unprefixed node modules. They will automatically be instrumented as prefixed and unprefixed. | ||
| '@aws/durable-execution-sdk-js': () => require('../aws-durable-execution-sdk-js'), |
There was a problem hiding this comment.
Would you mind moving this down to separate Node.js modules from the rest? :)
There was a problem hiding this comment.
Of course 🤦 completely missed this
| shimmer.massWrap(dp, ['then', 'catch', 'finally'], original => function (...args) { | ||
| attachSpy() |
There was a problem hiding this comment.
I think we could change the prototype of the promise and prevent rewrapping each promise afterwards. That should likely be more performant :)
| * @returns {boolean} | ||
| */ | ||
| function isReplayedOp (ctxImpl) { | ||
| const stepId = ctxImpl?.getNextStepId?.() |
There was a problem hiding this comment.
I believe the getNextStepId method is currently called twice in many cases where we could unify it to a single call for performance
| const contextMethods = [ | ||
| 'step', | ||
| 'invoke', | ||
| 'runInChildContext', | ||
| 'wait', | ||
| 'waitForCondition', | ||
| 'waitForCallback', | ||
| 'createCallback', | ||
| 'map', | ||
| 'parallel', | ||
| ] |
There was a problem hiding this comment.
Could we reuse this list to prevent the duplication? :)
…point.js Co-authored-by: Ruben Bridgewater <[email protected]>
…/aws-durable-execution-sdk-js # Conflicts: # packages/dd-trace/test/plugins/versions/package.json
Persist the current trace context as a synthetic `_datadog_{N}` STEP operation
when the SDK suspends to PENDING, so subsequent invocations (read by the
upstream datadog-lambda-js wrapper) can resume the same trace.
---------
Co-authored-by: Ruben Bridgewater <[email protected]>
Co-authored-by: Claude Opus 4.8 <[email protected]>
…/aws-durable-execution-sdk-js # Conflicts: # packages/dd-trace/test/plugins/versions/package.json
…/aws-durable-execution-sdk-js # Conflicts: # packages/dd-trace/test/plugins/versions/package.json
* workflow(aws-durable-execution-sdk-js): install_package * workflow(aws-durable-execution-sdk-js): generate_app * workflow(aws-durable-execution-sdk-js): compile * workflow(aws-durable-execution-sdk-js): test:att1:iter1:fixer * workflow(aws-durable-execution-sdk-js): test:att1:iter2:fixer * workflow(aws-durable-execution-sdk-js): feature_implement * workflow(aws-durable-execution-sdk-js): get_lint_failures * workflow(aws-durable-execution-sdk-js): lint_and_fix:att1:iter1:fix_lint_errors * workflow(aws-durable-execution-sdk-js): review_cycle:att1:iter1:batch_fix * remove the unnecessary dd-api-key * clean up # Conflicts: # index.d.ts * yarn.lock changed... * fixing yarn.lock * remove the unintended finish() guard * update span names * use a fixed service name instead * update resource names * naming consistency * small fix * Python PR parity * Undo unnecessary changes * Finish error spans on asyncEnd * Simplify orchestrion file * Class/file name changes * Several simplifications and improvements * Do not explicitly set component * Remove includeReplayedTag * Smaller simplifications * Tests simplification * chore: update supported-integrations * More test simplification * Add aws.durable.operation_id and aws.durable.operation_name * Fix checks * Linter * Test simplifications * More test improvements * Lazy thenables + only close this integration's spans * Code simplifications * Fix rebase * Mirror changes in v5 * Test waitForCondition happy path * Comment improvements based on guidelines * supress child context for WaitForCallback * Increase tested version * Address review comments * Avoid patching on the plugin by creating a "settle" channel * Do not skipTime to avoid interfering with tracer's timers * Fix test flakiness * Test durable-execution-sdk-js only on node >=22 * Linter * Rename context variables * Lint * Add esbuild bundling acceptance test * ESM smoke tests * Update packages/datadog-plugin-aws-durable-execution-sdk-js/src/handler.js Co-authored-by: Ruben Bridgewater <[email protected]> * Update packages/datadog-plugin-aws-durable-execution-sdk-js/src/checkpoint.js Co-authored-by: Ruben Bridgewater <[email protected]> * Address various review comments * Operation name * feat(aws): cross-invocation tracecontext propagation (#8182) Persist the current trace context as a synthetic `_datadog_{N}` STEP operation when the SDK suspends to PENDING, so subsequent invocations (read by the upstream datadog-lambda-js wrapper) can resume the same trace. --------- Co-authored-by: Ruben Bridgewater <[email protected]> Co-authored-by: Claude Opus 4.8 <[email protected]> --------- Co-authored-by: Pablo Martínez Bernardo <[email protected]> Co-authored-by: dd-octo-sts[bot] <200755185+dd-octo-sts[bot]@users.noreply.github.com> Co-authored-by: pablomartinezbernardo <[email protected]> Co-authored-by: Ruben Bridgewater <[email protected]> Co-authored-by: Claude Opus 4.8 <[email protected]>
* workflow(aws-durable-execution-sdk-js): install_package * workflow(aws-durable-execution-sdk-js): generate_app * workflow(aws-durable-execution-sdk-js): compile * workflow(aws-durable-execution-sdk-js): test:att1:iter1:fixer * workflow(aws-durable-execution-sdk-js): test:att1:iter2:fixer * workflow(aws-durable-execution-sdk-js): feature_implement * workflow(aws-durable-execution-sdk-js): get_lint_failures * workflow(aws-durable-execution-sdk-js): lint_and_fix:att1:iter1:fix_lint_errors * workflow(aws-durable-execution-sdk-js): review_cycle:att1:iter1:batch_fix * remove the unnecessary dd-api-key * clean up # Conflicts: # index.d.ts * yarn.lock changed... * fixing yarn.lock * remove the unintended finish() guard * update span names * use a fixed service name instead * update resource names * naming consistency * small fix * Python PR parity * Undo unnecessary changes * Finish error spans on asyncEnd * Simplify orchestrion file * Class/file name changes * Several simplifications and improvements * Do not explicitly set component * Remove includeReplayedTag * Smaller simplifications * Tests simplification * chore: update supported-integrations * More test simplification * Add aws.durable.operation_id and aws.durable.operation_name * Fix checks * Linter * Test simplifications * More test improvements * Lazy thenables + only close this integration's spans * Code simplifications * Fix rebase * Mirror changes in v5 * Test waitForCondition happy path * Comment improvements based on guidelines * supress child context for WaitForCallback * Increase tested version * Address review comments * Avoid patching on the plugin by creating a "settle" channel * Do not skipTime to avoid interfering with tracer's timers * Fix test flakiness * Test durable-execution-sdk-js only on node >=22 * Linter * Rename context variables * Lint * Add esbuild bundling acceptance test * ESM smoke tests * Update packages/datadog-plugin-aws-durable-execution-sdk-js/src/handler.js Co-authored-by: Ruben Bridgewater <[email protected]> * Update packages/datadog-plugin-aws-durable-execution-sdk-js/src/checkpoint.js Co-authored-by: Ruben Bridgewater <[email protected]> * Address various review comments * Operation name * feat(aws): cross-invocation tracecontext propagation (#8182) Persist the current trace context as a synthetic `_datadog_{N}` STEP operation when the SDK suspends to PENDING, so subsequent invocations (read by the upstream datadog-lambda-js wrapper) can resume the same trace. --------- Co-authored-by: Ruben Bridgewater <[email protected]> Co-authored-by: Claude Opus 4.8 <[email protected]> --------- Co-authored-by: Pablo Martínez Bernardo <[email protected]> Co-authored-by: dd-octo-sts[bot] <200755185+dd-octo-sts[bot]@users.noreply.github.com> Co-authored-by: pablomartinezbernardo <[email protected]> Co-authored-by: Ruben Bridgewater <[email protected]> Co-authored-by: Claude Opus 4.8 <[email protected]>
* workflow(aws-durable-execution-sdk-js): install_package * workflow(aws-durable-execution-sdk-js): generate_app * workflow(aws-durable-execution-sdk-js): compile * workflow(aws-durable-execution-sdk-js): test:att1:iter1:fixer * workflow(aws-durable-execution-sdk-js): test:att1:iter2:fixer * workflow(aws-durable-execution-sdk-js): feature_implement * workflow(aws-durable-execution-sdk-js): get_lint_failures * workflow(aws-durable-execution-sdk-js): lint_and_fix:att1:iter1:fix_lint_errors * workflow(aws-durable-execution-sdk-js): review_cycle:att1:iter1:batch_fix * remove the unnecessary dd-api-key * clean up # Conflicts: # index.d.ts * yarn.lock changed... * fixing yarn.lock * remove the unintended finish() guard * update span names * use a fixed service name instead * update resource names * naming consistency * small fix * Python PR parity * Undo unnecessary changes * Finish error spans on asyncEnd * Simplify orchestrion file * Class/file name changes * Several simplifications and improvements * Do not explicitly set component * Remove includeReplayedTag * Smaller simplifications * Tests simplification * chore: update supported-integrations * More test simplification * Add aws.durable.operation_id and aws.durable.operation_name * Fix checks * Linter * Test simplifications * More test improvements * Lazy thenables + only close this integration's spans * Code simplifications * Fix rebase * Mirror changes in v5 * Test waitForCondition happy path * Comment improvements based on guidelines * supress child context for WaitForCallback * Increase tested version * Address review comments * Avoid patching on the plugin by creating a "settle" channel * Do not skipTime to avoid interfering with tracer's timers * Fix test flakiness * Test durable-execution-sdk-js only on node >=22 * Linter * Rename context variables * Lint * Add esbuild bundling acceptance test * ESM smoke tests * Update packages/datadog-plugin-aws-durable-execution-sdk-js/src/handler.js Co-authored-by: Ruben Bridgewater <[email protected]> * Update packages/datadog-plugin-aws-durable-execution-sdk-js/src/checkpoint.js Co-authored-by: Ruben Bridgewater <[email protected]> * Address various review comments * Operation name * feat(aws): cross-invocation tracecontext propagation (#8182) Persist the current trace context as a synthetic `_datadog_{N}` STEP operation when the SDK suspends to PENDING, so subsequent invocations (read by the upstream datadog-lambda-js wrapper) can resume the same trace. --------- Co-authored-by: Ruben Bridgewater <[email protected]> Co-authored-by: Claude Opus 4.8 <[email protected]> --------- Co-authored-by: Pablo Martínez Bernardo <[email protected]> Co-authored-by: dd-octo-sts[bot] <200755185+dd-octo-sts[bot]@users.noreply.github.com> Co-authored-by: pablomartinezbernardo <[email protected]> Co-authored-by: Ruben Bridgewater <[email protected]> Co-authored-by: Claude Opus 4.8 <[email protected]>
6 participants
Summary
Adds automatic instrumentation for the
@aws/durable-execution-sdk-jslibrary.Traced Operations
withDurableExecution()wrapperDurableContext.step()DurableContext.invoke()DurableContext.wait()DurableContext.waitForCondition()DurableContext.waitForCallback()DurableContext.createCallback()DurableContext.map()DurableContext.parallel()DurableContext.runInChildContext()Implementation Notes
Lazy DurablePromise observation
DurableContextmethods returnDurablePromiseinstances that delay the actual execution until user code first awaits/chains them. The integration useskind: 'Sync'and anobserveDurablePromisehelper that wrapsthen/catch/finally. The actual span-finishing chain only attaches after user code first awaits.Suspended (PENDING) workflows
When execution is suspended, and the SDK intentionally leaves the suspended operation's
DurablePromiseneither resolved nor rejected. So the handler plugin detectsPENDINGand finishes any still-open child spans owned by this integration before returning, so the trace processor can flush the partial trace.Checkpoint-driven retry finalization
On a step retry, the SDK doesn't reject the step's
DurablePromise, it suspends execution and writes a checkpoint withAction: 'RETRY'. TheCheckpointManager.checkpointhook detects that, stampserror.{type,message,stack}on the still-open span using the checkpoint's error payload, and finishes the span onasyncEnd.map/parallel resource cardinality + child_context suppression
For
map()/parallel()/waitForCallback()the SDK callsrunInChildContextwith internalsubTypes (Map,Parallel,MapIteration,ParallelBranch,WaitForCallback). Those internal spans are suppressed. Resource is also removed from direct step children ofaws.durable.map/aws.durable.parallelto avoid unbounded resource cardinality from per-iteration operation names.