Skip to content

chore(logging): adopt AWS Lambda Powertools structured logging - #464

Open
jeromevdl wants to merge 3 commits into
mainfrom
chore/powertools-logging
Open

jeromevdl wants to merge 3 commits into
mainfrom
chore/powertools-logging

Conversation

@jeromevdl

@jeromevdl jeromevdl commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Migrates the entire Lambda codebase (plus shared modules and the agentcore/yjs container runtimes) from raw console.* to AWS Lambda Powertools Logger, producing structured JSON logs with consistent correlation context.

Logging convention (three layers)

  • servicecollaborative-aidlc, set once via POWERTOOLS_SERVICE_NAME (a Terraform local referenced by all 28 Lambda blocks + the AgentCore and yjs container runtimes), never hardcoded in constructors.
  • component — the deployable unit (workspace name), as a persistentKeys entry.
  • module — subsystem, for files that previously carried a [prefix] tag (e.g. both MCP files → module: 'mcp').

Correlation context

  • appendKeys + resetKeys add request-scoped ids (intentId/projectId/taskId/userId) on the orchestration-heavy handlers (intents, projects, agents) and v2-orchestrator (migrated off the durable-SDK ctx.logger).

Event logging (opt-in)

  • Central POWERTOOLS_LOGGER_LOG_EVENT bool (default false) wired to all Lambda blocks; logEventIfEnabled(event) on the API-GW handlers. The git OAuth-callback paths are excludedgit-handler.js keeps its bespoke redacted Request log so enabling event logging can't leak secrets.

Failure visibility

  • New logging at the agentcore dispatch boundary (http-server.js: unknown/missing command, any { ok: false } result, any throw) and the orchestrator runtime-invoke path (defaultInvokeRuntime), so silent failures (e.g. a runtime error returned as a non-throwing body) surface in CloudWatch with command + reason/detail/error.

Correctness fixes surfaced during review

  • Restored per-level intent on injected log sinks (info vs error), normalized error logging to the positional logger.error(msg, err, {ctx}) shape, stripped redundant [component] message prefixes.

Verification

  • Full suite: 2834 passed, 3 skipped, 0 failures; lint 0 errors; oxfmt + terraform fmt clean.
  • Powertools confirmed working in the deployed agentcore container (structured log lines observed live).

Follow-ups (not in this PR)

  • Remove the test-only injected log param seam in the agentcore commands (TODOs in place).

Covers #281 partially

@jeromevdl jeromevdl changed the title chore(lambda): adopt AWS Lambda Powertools structured logging chore(logging): adopt AWS Lambda Powertools structured logging Sep 14, 2026

@JWThewes JWThewes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The structured logging migration is a reasonable direction, but I found two material regressions: MCP diagnostics are written into the protocol stream, and enabling event logging exposes credentials. Please address the two inline comments before merging.

Validation: all 2,837 backend tests passed locally, and all Lambda bundles built successfully. Separate before/after probes reproduced both issues on this commit and confirmed they are absent from the base commit.

Comment thread lambda/agentcore/mcp/server.js Outdated
console.error(
`[mcp-trace] ${name} ok=${!env?.isError} bytes=${bytes} ms=${Date.now() - startedAt} args=${Object.keys(args ?? {}).join(',')}`,
);
logger.info('mcp-trace', {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Keep MCP diagnostics on stderr

At the default INFO level, this writes a Powertools JSON log to stdout. This module runs behind StdioServerTransport, so stdout is the MCP JSON-RPC stream consumed by the agent CLI. Each traced tool call now emits a non-protocol message, and the new startup log in mcp/index.js:77 has the same problem. I reproduced a ZodError using the actual MCP SDK stdio client against the entrypoint; the base commit produces no protocol errors. This can break initialization or tool calls in clients that reject malformed protocol output. Route all diagnostics emitted by the MCP process to stderr and add a regression test through the real stdio transport.

Comment thread lambda/agents/index.js Outdated
export const handler = async (event, context) => {
if (context) logger.addContext(context);
logger.resetKeys();
logger.logEventIfEnabled(event);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Redact credentials before logging API events

logEventIfEnabled(event) logs the entire API Gateway event without redaction. With POWERTOOLS_LOGGER_LOG_EVENT=true, I reproduced a successful PUT /agents/settings that writes the raw Bedrock bearer token, Kiro API key, and Authorization header to the log. The base commit logs none of these values. The same full-event logging approach also covers project MCP-secret updates and tracker OAuth callbacks, so excluding git-handler.js does not preserve the credential-redaction boundary. Although the flag defaults to false, enabling diagnostics should not disclose credentials. Log allowlisted request metadata or redact sensitive headers, bodies, and query parameters before logging, and cover the credential-bearing routes with regression tests.

@jeromevdl

Copy link
Copy Markdown
Contributor Author

Addressed both review findings:

  • MCP stdio diagnostics now stay on stderr, with a real SDK stdio transport regression test.
  • API event logging now redacts credentials in headers, OAuth parameters, agent settings, MCP secrets, and environment-variable maps, with regression coverage.
  • Powertools Terraform configuration is centralized and completed for the intended REST handlers.

Validation: 2,856 backend tests pass; lint, formatting, and Terraform validation pass.

@jeromevdl
jeromevdl force-pushed the chore/powertools-logging branch from f7b45ed to abdfc5c Compare September 15, 2026 21:51

@JWThewes JWThewes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed the latest changes. The MCP stdout issue is addressed: diagnostics use stderr again, and the real stdio transport regression test passes. The original Bedrock, Kiro, and Authorization values are also now redacted.

One blocker remains: credential redaction is incomplete, as detailed in the inline comment. I reproduced the remaining leaks using synthetic credentials in a successful settings update.

Validation: all 2,945 backend tests passed locally, all Lambda bundles built successfully, and CI is green.

if (normalized === 'ticket' && route.includes('/trackers/connections/')) {
return [key, REDACTED];
}
if (inMcpConfig && (normalized === 'headers' || normalized === 'env')) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Complete credential redaction before logging API events

The MCP branch masks env and headers, but leaves url and args unchanged. With POWERTOOLS_LOGGER_LOG_EVENT=true, I reproduced a successful PUT /agents/settings using configurations accepted by validateMcpServersJson where API keys in a remote URL (?api_key=...) and command arguments (--api-key ...) both reach the log.

The same request also logs X-Origin-Verify unchanged through redactNamedValues. CloudFront injects this deployment secret into API requests, and the optional API origin policy uses it to verify that traffic came through CloudFront. It needs protection in both headers and multiValueHeaders.

The original Bedrock, Kiro, and Authorization fields are correctly redacted now, but these remaining cases keep the credential-logging finding open. For simplicity and maintainability, prefer allowlisted request metadata rather than an expanding credential blacklist. Alternatively, redact entire MCP configurations and add the origin-verification header to the sensitive headers. Please cover these cases with regression tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants