Conversation
Sushisource
force-pushed
the
lambda-worker
branch
2 times, most recently
from
3ecf038 to
8040631
Compare
Sushisource
force-pushed
the
lambda-worker
branch
from
8040631 to
6fe3cbb
Compare
chris-olszewski
left a comment
chris-olszewski
left a comment
There was a problem hiding this comment.
Overall looks good, just a few concerns around the otel code configuration setup.
| case 'ERROR': | ||
| this.ptLogger.error(message, meta as Record<string, unknown>); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Not that I ever expect us to add a new log level
| } | |
| default: | |
| level satisfies never; | |
| break; | |
| } |
packages/lambda-worker/README.md
Outdated
|
|
||
| export const handler = runWorker({ deploymentName: 'my-service', buildId: 'v1.0' }, (config) => { | ||
| config.workerOptions.taskQueue = 'my-task-queue'; | ||
| config.workerOptions.workflowBundle = require('./workflow-bundle.json'); |
There was a problem hiding this comment.
| config.workerOptions.workflowBundle = require('./workflow-bundle.json'); | |
| config.workerOptions.workflowBundle = { code: require('./workflow-bundle.js') }; |
packages/lambda-worker/src/types.ts
Outdated
| * You must set at least `taskQueue` (or set the `TEMPORAL_TASK_QUEUE` env var). Typically you'll | ||
| * also set `activities` and `workflowBundle` (prefer pre-bundled workflows over `workflowsPath` | ||
| * to avoid bundling overhead on Lambda cold starts). | ||
| * | ||
| * The following fields are managed by settings elsewhere and will be overridden per-invocation: | ||
| * `connection`, `namespace`, `identity`, `workerDeploymentOptions`. | ||
| */ | ||
| workerOptions: Partial<WorkerOptions>; |
There was a problem hiding this comment.
Not sure how beneficial this would be, but could convey that connection etc aren't respected via the type
| * You must set at least `taskQueue` (or set the `TEMPORAL_TASK_QUEUE` env var). Typically you'll | |
| * also set `activities` and `workflowBundle` (prefer pre-bundled workflows over `workflowsPath` | |
| * to avoid bundling overhead on Lambda cold starts). | |
| * | |
| * The following fields are managed by settings elsewhere and will be overridden per-invocation: | |
| * `connection`, `namespace`, `identity`, `workerDeploymentOptions`. | |
| */ | |
| workerOptions: Partial<WorkerOptions>; | |
| * You must set at least `taskQueue` (or set the `TEMPORAL_TASK_QUEUE` env var). Typically you'll | |
| * also set `activities` and `workflowBundle` (prefer pre-bundled workflows over `workflowsPath` | |
| * to avoid bundling overhead on Lambda cold starts). | |
| * | |
| * The following fields are managed by settings elsewhere and will be overridden per-invocation: | |
| * `connection`, `namespace`, `identity`, `workerDeploymentOptions`. | |
| */ | |
| workerOptions: Partial<Omit<WorkerOptions, 'connection' | 'namespace' | 'identity' | 'workerDeploymentOptions'>>; |
| config.workerOptions.taskQueue = envTaskQueue; | ||
| } | ||
|
|
||
| const connectConfig = deps.loadConnectConfig(config.envConfigOptions); |
There was a problem hiding this comment.
config.envConfigOptions will always be undefined at this point
| workerDeploymentOptions: { | ||
| version, | ||
| useWorkerVersioning: true, | ||
| defaultVersioningBehavior: 'PINNED', | ||
| }, |
There was a problem hiding this comment.
Do we want to allow this to get modified by the user? If not, we should probably have a userConfig object that gets passed in and then later fully resolved into the config required to run the serverless worker.
There was a problem hiding this comment.
We do. They should be able to change the default behavior. They can't set useWorkerVersioning to false, though, and that's enforced at runtime. I can probably enforce that at compile time though so I'll see about doing that.
| connection, | ||
| namespace, | ||
| identity, | ||
| } as WorkerOptions; |
There was a problem hiding this comment.
Would feel better if this was a smaller cast than it currently is. I think the connection is the only type that actually needs massaging. Or beef up the connection mock definition, but that might be overkill.
| new Promise<void>((resolve) => { | ||
| setTimeout(resolve, workTimeMs); | ||
| }) |
There was a problem hiding this comment.
Could use the stdlib promise timers: https://nodejs.org/api/timers.html#timerspromisessettimeoutdelay-value-options
| new Promise<void>((resolve) => { | |
| setTimeout(resolve, workTimeMs); | |
| }) | |
| setTimeout(workTimeMs) |
| * { buildId: 'v1.0.0', deploymentName: 'my-service' }, | ||
| * (config) => { | ||
| * config.workerOptions.taskQueue = 'my-task-queue'; | ||
| * config.workerOptions.workflowBundle = require('./workflow-bundle.json'); |
There was a problem hiding this comment.
| * config.workerOptions.workflowBundle = require('./workflow-bundle.json'); | |
| * config.workerOptions.workflowBundle = { code: require('./workflow-bundle.js') }; |
packages/lambda-worker/src/otel.ts
Outdated
| * @example | ||
| * ```ts | ||
| * import { runWorker } from '@temporalio/lambda-worker'; | ||
| * import { applyDefaults } from '@temporalio/lambda-worker/lib/otel'; |
There was a problem hiding this comment.
We should re-export these to as an otel module to avoid having users reach into lib. I think if you make otel it's own package entry point you could avoid the dynamic imports as well.
There was a problem hiding this comment.
Let me know if what I did is what you're thinking of
3 participants
Temporal workers inside AWS Lambda (connect, create worker, poll, graceful shutdown)
logging via Powertools, and optional OTEL helpers for ADOT integration
handling, connection lifecycle, and connection options