Skip to content

Carry structured logging properties through worker hooks - #857

Open
Radek Maziarka (rmaziarka) wants to merge 2 commits into
Azure:v3.xfrom
rmaziarka:v3.x
Open

Radek Maziarka (rmaziarka) wants to merge 2 commits into
Azure:v3.xfrom
rmaziarka:v3.x

Conversation

@rmaziarka

@rmaziarka Radek Maziarka (rmaziarka) commented Sep 29, 2026 •

Copy link
Copy Markdown

Project

Problem

Proposed solution

  • Optional metadata argument on the existing core logger; no new public logging method.
  • Explicit worker capability for structured properties.
  • Separate properties on LogHookContext; unchanged formatted message and invocation context.
  • Consumer registration indicating support for structured properties.
  • Missing or outdated consumer: retain the existing text log and emit a bounded warning.
  • Flat scalar validation; reserved-key protection; no new application exception from metadata processing.
  • In-process transport to hooks; no host or protobuf changes.
  • Tests: property preservation, concurrent invocation isolation, compatibility and missing-consumer behavior.
  • Release core types and worker support before PRs 2 and 3 are activated.

Validation

  • Warning limits use a stable invocation scope from creation through execution; regression coverage includes logging in getInvocationModel, getArguments and invokeFunction.

  • Structured logging options are accepted only for the log-hook overload.

  • 159 tests passed; 8 existing tests for older Node versions skipped on Node 24. Build and lint passed.

  • Combined integration test: local Functions host, modified packages and live Application Insights; 30 records verified across CommonJS, ESM, disabled opt-in, missing consumer and old-worker scenarios.

  • Verified message preservation, custom dimensions, severity and invocation correlation. Not tested in managed Azure Functions; rollout requires coordinated releases.

Copilot AI balanced review requested due to automatic review settings September 29, 2026 09:14
@rmaziarka
Radek Maziarka (rmaziarka) requested a review from a team as a code owner September 29, 2026 09:14
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

It changes a shared critical-path logging contract and still has unresolved warning-scope and API-overload issues requiring code-owner review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Adds opt-in structured logging metadata to the worker’s core API and carries validated attributes to process-local log hooks without changing Host RPC messages.

Changes:

  • Extends core logging and hook APIs with structured attributes and capability detection.
  • Sanitizes attributes, protects reserved keys, and emits bounded compatibility warnings.
  • Adds compatibility, concurrency, disposal, validation, and transport tests.

Human Review Packet

HUMAN REVIEW REQUIRED

Gate: A - before implementation
Owner: @azure/azure-functions-nodejs
Decision needed: Approve the new cross-repository core logging contract, compatibility behavior, and coordinated rollout.

Critical paths:

  • WorkerContext, InvocationHandler, core-module setup, and hooks alter invocation logging and public programming-model behavior.

Behavioral contract:

  • Before: Logs exposed only formatted messages to hooks and Host RPC.
  • After: Opted-in hooks receive sanitized structured attributes through an extended core API.
  • Must remain invariant: Existing message, severity, correlation, hook mutation, and Host RPC behavior.

Blast radius:

  • Programming-model and exporter compatibility, concurrent invocations, specialization, and coordinated package releases.

Security analysis:

  • No evidence-backed vulnerability found; attributes remain process-local and reserved or invalid values are filtered.

Validation evidence:

  • NOT RUN: Independent repository checks were unavailable under review constraints; the PR reports 157 tests, lint, build, and local Host integration passing.

Residual risks or open decisions:

  • Invocation warning scope is incorrect before logContext initialization, the generic hook overload accepts ignored options, and managed Azure validation is absent.

Human decision: PENDING

File Description
types-core/​index.d.ts Extends logging and hook types.
src/​AppContext.ts Tracks structured-capable hooks.
src/​WorkerContext.ts Sanitizes metadata and emits warnings.
src/​setupCoreModule.ts Exposes capability detection.
src/​hooks/​registerHook.ts Records consumer support.
src/​hooks/​LogHookContext.ts Exposes sanitized attributes.
src/​eventHandlers/​InvocationHandler.ts Carries invocation metadata.
src/​coreApi/​coreApiLog.ts Carries application-level metadata.
src/​utils/​structuredLogProperties.ts Validates and freezes attributes.
test/​eventHandlers/​structuredLogging.test.ts Tests structured logging behavior.
README.md Documents the new contract.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/WorkerContext.ts Outdated
reason: string,
rejectedCount?: number
): void {
const scope = invocationLogCtx ?? this.app;
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