feat(insight): add ten Workflow Insight exporters - #704
Conversation
|
/ai review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
360348e to
31041e6
Compare
This comment has been minimized.
This comment has been minimized.
- DynamoDBExporter: PutItem keyed by execution ARN, optional emittedAt sort key, by-name rendering - AuroraExporter: RDS Data API upsert per execution, PostgreSQL or MySQL dialect - OTelExporter: one OTLP/HTTP JSON log record per insight record - FirehoseExporter: one newline-terminated JSON record per PutRecord - EventBridgeExporter: one PutEvents entry, status as DetailType - RedshiftExporter: Redshift Data API MERGE per execution - OpenSearchExporter: index API PUT with SigV4 or basic auth - SQSExporter: SendMessage with FIFO group and deduplication ids - HttpExporter: POST or PUT JSON with timeout - FileExporter: NDJSON append or one pretty JSON file per execution - OperationsFormat (ARRAY, BY_NAME, BOTH) for flexible destinations - Optional SDK clients are created on first use; a missing artifact fails with a message naming it
31041e6 to
c66fb37
Compare
| HttpRequest.Builder rb = HttpRequest.newBuilder(endpoint) | ||
| .POST(HttpRequest.BodyPublishers.ofString(body, StandardCharsets.UTF_8)) | ||
| .setHeader("Content-Type", "application/json"); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_m6wtujrjs3jdqrxjixqe66bfyh
[P1] Bound these HTTP requests and the equivalent OpenSearch request. Neither client has a connection or request timeout, and onInvocationEnd() drains exporter workers, so an unresponsive peer can hold the Lambda invocation until its own timeout. Add validated timeout options, configure the clients, apply HttpRequest.timeout(), and test slow endpoints.
| redshift.executeStatement(ExecuteStatementRequest.builder() | ||
| .workgroupName(workgroupName) | ||
| .clusterIdentifier(clusterIdentifier) | ||
| .database(database) | ||
| .dbUser(dbUser) | ||
| .secretArn(secretArn) | ||
| .sql(buildMerge(execNameSel, startTimeSel, endTimeSel, durationSel)) | ||
| .parameters(parameters) | ||
| .build()); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_zjxpfje7ptkmqdr7dzlabjykge
[P1] Wait for the submitted statement to finish. Redshift Data API's executeStatement returns after queueing asynchronous work, so later SQL or permission failures are lost; an older RUNNING merge can also finish after the terminal merge and overwrite it. Poll DescribeStatement to a terminal status with a bounded timeout, surface failures, document the additional IAM action, and add ordering/failure tests.
| synchronized (appendLock) { | ||
| Files.write( | ||
| directory.resolve(date + ".ndjson"), | ||
| line, | ||
| StandardOpenOption.CREATE, | ||
| StandardOpenOption.APPEND); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_amm73zbcbjs32xy6zfgxauxtdh
[P1] This in-memory lock does not protect the advertised EFS use case. Concurrent Lambda execution environments have separate FileExporter instances and locks, so their multi-chunk appends to the shared date file can interleave and corrupt NDJSON. Hold a cross-process FileChannel lock for the complete write or use per-execution files, and test independent writers.
| headers.forEach(rb::setHeader); | ||
| HttpResponse<Void> response; | ||
| try { | ||
| response = httpClient.send(rb.build(), HttpResponse.BodyHandlers.discarding()); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_qiq3jx77pc5jjxmdbtj5gzu5ho
[P2] Do not discard a successful OTLP response body. A 2xx response may contain partialSuccess.rejectedLogRecords, including rejection of this sole record, but this reports delivery as successful. Read and parse the response, fail when any record was rejected, and add a partial-success test.
| String name = record.executionName() != null ? record.executionName() : record.executionArn(); | ||
| Files.write( | ||
| directory.resolve(sanitize(name) + ".json"), |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_nbydgvplnwukj4ma2x45s3gadq
[P2] Derive JSON filenames from the globally unique execution ARN. Execution names can repeat across functions, and sanitization also maps distinct names to the same filename, causing one execution's record to overwrite another. Include an ARN-derived hash or encoded ARN and test both collision cases.
| item.put(partitionKey, record.executionArn()); | ||
| if (sortKey != null) { | ||
| item.put(sortKey, item.get("emittedAt")); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_qyyvkur5ncyvig2l4sf3mbd66h
[P2] Reject identical partition and sort key names. With both configured to the same attribute, the sort-key assignment overwrites the execution ARN with emittedAt, silently changing the item's identity. Validate that enabled key names are distinct at build time and add a rejection test.
| public Object render(WorkflowInsightRecord record) { | ||
| return operationsFormat.apply(record); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_n2zzkjbealsdehyxh3k6ponc4o
[P2] Make render() measure the representation JSON mode actually writes. Truncation currently measures compact JSON, while export emits larger pretty-printed JSON, so a configured maxRecordSizeBytes can still be exceeded. Measure the pretty serialization or write the compact representation when enforcing a limit, and add a near-limit JSON-mode test.
Codex AI reviewFound seven actionable delivery and correctness issues across the new exporters. Reviewed commit |
|
OpenSearchExporter.build() throws NoClassDefFoundError when the auth artifact is absent, in both auth modes.
=== auth removed
Fix: hold the provider in an Object field and cast inside Signer, so only Signer's constructor resolves the type. Then add OpenSearchExporter to OptionalArtifactTest with auth and http-auth-aws each removed. With auth present and only http-auth-aws removed, the mechanism does work: SIGV4 failed at export with Missing dependency software.amazon.awssdk:http-auth-aws, and BASIC exported normally. |
|
Confirmed and reproduced (same probe shape: child-first loader with |
2 participants
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Issue Link, if available
N/A
Description
Adds ten Workflow Insight exporters to
insight-plugin:DynamoDBExporter,AuroraExporter,OTelExporter,FirehoseExporter,EventBridgeExporter,RedshiftExporter,OpenSearchExporter,SQSExporter,HttpExporter, andFileExporter, each behind a builder with defaults matching the other SDKs' insight plugins. A sharedOperationsFormat(ARRAY,BY_NAME,BOTH) selects the operations rendering for flexible destinations. AWS SDK clients are created on first use throughLazyClient, so a missing optional artifact fails at export with a message naming it and the plugin keeps the other exporters running;S3ExporterandCloudWatchLogsExporternow use the same path. New AWS SDK artifacts in the pom are all<optional>true</optional>; no new required dependency.Demo/Screenshots
N/A
Checklist
Testing
Unit Tests
One test class per exporter under
insight-plugin/src/test, asserting exact request fields and bodies through mocked clients (ArgumentCaptor), a loopbackHttpServerfor OTel/HTTP/OpenSearch (method, headers, body, timeout, SigV4Authorizationpresence, basic-auth encoding), and@TempDirforFileExporter(both modes, append semantics, concurrent appends).OperationsFormatTestandLazyClientTestcover the shared helpers.mvn -pl insight-plugin clean install: 141 tests pass.OptionalArtifactTestbuilds each SDK exporter through a class loader with its service jar removed and asserts build succeeds and export fails naming the artifact. A fullmvn clean installpassed on the pre-rebase commit; after rebasing on #703 the reactor stopped on an unrelatedotel-plugintest (InvocationOtelPluginTest.invocationEnd_closesNestedSpansChildFirst) that passes when run alone.Integration Tests
Not applicable: exporters call external services and are covered by unit tests with mocked clients.
Examples
Not applicable. README documents each exporter's setup, artifact, IAM actions, and a minimal builder call.
Known follow-ups
WorkflowInsightRecordforemittedAt,endTime,durationMs,region,accountId,functionQualifier, andschemaVersion; exporters currently read these from the rendered wire map.OTelExportersupportshttp/jsononly;http/protobufis rejected at build time.OTelExportertreats any 2xx as delivered and does not parsepartialSuccess.rejectedLogRecords.OTelExporterandOpenSearchExporterhave no request timeout option; adding one is a cross-SDK config decision.RedshiftExportersubmits the statement without waiting for completion (best-effort, by design); later SQL failures are not surfaced.