fix: remove await from OTEL handleSignal interceptor #1803
Conversation
chris-olszewski
force-pushed
the
olszewski/fix_otel_interceptor_nde
branch
from
717716c to
1006b2b
Compare
chris-olszewski
force-pushed
the
olszewski/fix_otel_interceptor_nde
branch
from
77b9399 to
dac7f6c
Compare
|
|
||
| async execute(input: ActivityExecuteInput, next: Next<ActivityInboundCallsInterceptor, 'execute'>): Promise<unknown> { | ||
| const context = await extractContextFromHeaders(input.headers); | ||
| const context = extractContextFromHeaders(input.headers); |
There was a problem hiding this comment.
Unconditionally removed this await since we cannot check sdk flags in this context. I was not able to trigger any NDE with this await being present/not present. I believe this removal is safe, but can add a Promise.resolve to keep the yield point if we think that would be safer.
There was a problem hiding this comment.
I don't really know the implications of this change, why do you think this is safe?
My immediate intuition is that it'll causing different microtask queuing.
There was a problem hiding this comment.
It will cause different microtask queueing, but I suppose the question is if that change will be impactful. I can add a synthesized yield point back here to be extra cautious
There was a problem hiding this comment.
Yeah I'm not sure - my feeling is that less disruption would be better, but @mjameswh would be better to ask
packages/interceptors-opentelemetry/src/workflow/workflow-module-loader.ts
Show resolved
Hide resolved
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| const { SdkFlags } = require('@temporalio/workflow/lib/flags'); | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| const { getActivator } = require('@temporalio/workflow/lib/global-attributes'); |
There was a problem hiding this comment.
Why do we need to import in the function?
Generally - maybe this indicates the function belongs in @temporalio/workflow/lib and we just need a single internal import to use this (instead of 3). This file could just re-export it, or import directly in the index.ts usage, not sure.
There was a problem hiding this comment.
This is an outcome of the recent PR to make @temporalio/workflow an optional peer dep for the interceptors: #1812, so we have to do some song and dance around importing from that package now that it might not be present.
From my understanding talking to @mjameswh we don't want to advertise these APIs for general consumption. With that in mind, I think keeping these in the nested modules is okay.
There was a problem hiding this comment.
Ah ok. But I feel as though if we're importing import type { SdkFlags as SdkFlagsT } from '@temporalio/workflow/lib/flags'; at the top anyway, it's a bit odd.
There was a problem hiding this comment.
Is it the type aliasing that is odd or the fact that we have a top level import and an import in a function?
|
|
||
| async execute(input: ActivityExecuteInput, next: Next<ActivityInboundCallsInterceptor, 'execute'>): Promise<unknown> { | ||
| const context = await extractContextFromHeaders(input.headers); | ||
| const context = extractContextFromHeaders(input.headers); |
There was a problem hiding this comment.
I don't really know the implications of this change, why do you think this is safe?
My immediate intuition is that it'll causing different microtask queuing.
|
Haven't read through the tests yet. |
| const { a, b, c } = workflow.proxyLocalActivities({ | ||
| scheduleToCloseTimeout: '1m', | ||
| }); |
There was a problem hiding this comment.
where are these activities coming from? I don't see any activities registered on the test workers, am i missing something?
There was a problem hiding this comment.
I did remove the code to create the histories since the tests only replay history to check for verify there isn't NDE. You can view the code used at abf1134
What was changed
Remove yield points from the
@temporalio/interceptors-opentelemetryworkflow interceptors. Some yield points were there since the initial version of the package and others were added in #1449 and #1577Why?
These yield points have the ability to cause NDE due to the inclusion of these interceptors. In addition, it broke
startWithSignaloperations as signal handlers were not immediately processed upon registration if the workflow was started with the given signal.Depends on temporalio/sdk-core#1040
Checklist
Closes [Bug] Upgrading OTel Interceptor to 1.11.5+ breaks signal-with-start and may cause NDE #1677
How was this tested:
Created histories from the affected versions and added tests that replay these histories. These are red/green tests where they initially failed without the changes, but with the flagging logic they passed.
The only interceptors that are not covered by these replay tests are:
executescheduleLocalActivitycontinueAsNewAs I was unable to trigger NDE from removing/adding yield points to these interceptors.
Possibly a warning on https://docs.temporal.io/develop/typescript/interceptors#workflow-interceptors-registration about adding yield points to workflow interceptors.