Skip to content

Fix: adapt BatchLogRecordProcessor to the OTel 0.220 options signature and bump OTel to 0.220 - #687

Open
AmaadMartin wants to merge 3 commits into
mainfrom
fix/otel-0220-batch-log-record-processor
Open

Fix: adapt BatchLogRecordProcessor to the OTel 0.220 options signature and bump OTel to 0.220#687
AmaadMartin wants to merge 3 commits into
mainfrom
fix/otel-0220-batch-log-record-processor

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 5, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    Related: chore: Bump the npm_and_yarn group across 1 directory with 4 updates google/adk-js#448
  2. Or, if no issue exists, describe the change:
    Problem: Advisory GHSA-8988-4f7v-96qf reports @opentelemetry/core < 2.8.0. The hoisted copy is already 2.8.0, but the four OTLP packages on 0.205 pin @opentelemetry/core at exactly 2.1.0, so six nested vulnerable copies remain and cannot be deduped or floated. Bumping those packages to 0.220 breaks the core build, because @opentelemetry/sdk-logs@0.220 replaced the BatchLogRecordProcessor positional exporter argument with a required options object. That break is why the grouped Dependabot PR chore: Bump the npm_and_yarn group across 1 directory with 4 updates google/adk-js#448 has been red since it was opened.

Solution: I pass the documented options object at core/src/telemetry/setup.ts:163 and bump only the OpenTelemetry packages. BatchLogRecordProcessorOptions.exporter is typed LogRecordExporter and OTLPLogExporter implements it, so no cast is needed and none is added. This PR supersedes the OpenTelemetry part of google#448; that PR is deliberately left open for a maintainer to decide on.

Detail:

  • Collision check. I listed all 379 open PRs on the fork and searched them for otel, opentelemetry, telemetry, 0.220, BatchLogRecordProcessor and sdk-logs. No open PR bumps the OpenTelemetry packages or touches core/src/telemetry/setup.ts. Feat: make @google-cloud/storage and the OTel GCP exporters optional peer dependencies #387 edits core/package.json, but only to move the three @google-cloud/* dependencies into peerDependencies; it does not touch the @opentelemetry/* ranges. This branch is therefore based on main, not stacked.

  • The upstream break, from the installed @opentelemetry/sdk-logs@0.220.0 declarations:

    // build/src/export/BatchLogRecordProcessorBase.d.ts
    export declare abstract class BatchLogRecordProcessorBase<
        T extends BatchLogRecordProcessorOptions> implements LogRecordProcessor {
      constructor(options: T);
    }
    // build/src/types.d.ts
    export interface BatchLogRecordProcessorOptions {
      exporter: LogRecordExporter;
      ...
    }

    On 0.205 the constructor was constructor(_exporter: LogRecordExporter, config?: BufferConfig).

  • Advisory evidence. npm audit against the pre-bump lockfile reports @opentelemetry/core moderate range: <2.8.0, via GHSA-8988-4f7v-96qf. After the bump @opentelemetry/core no longer appears in npm audit output, and no copy below 2.8.0 remains: one hoisted 2.8.0 plus eight nested 2.9.0. The six nested 2.1.0 copies are gone.

  • Deviation from the plan I worked to. That plan predicted every copy would end at 2.9.0. The hoisted copy stays at 2.8.0, because @opentelemetry/resources, sdk-metrics, sdk-trace-base and sdk-trace-node are locked at 2.8.0 and npm does not float them on this bump. Floating them pulls in 2.10.0 and an extra nested @opentelemetry/sdk-trace, which is out of scope, so I reverted it. 2.8.0 is the advisory's first patched version, so the advisory is clear either way.

  • Why @opentelemetry/api-logs is here even though Fix: honor a caller-supplied appName in the Agent Engine deploy path (and expose --app_name) #448 does not bump it: sdk-logs@0.220.0, exporter-logs-otlp-http@0.220.0 and otlp-transformer@0.220.0 each declare "@opentelemetry/api-logs": "0.220.0" exactly. A ^0.205.0 range does not admit 0.220.0, so leaving it behind installs a second copy of the logs API. LogsAPI keeps its provider in a module-level singleton, so two copies mean two global logger registries and logs.setGlobalLoggerProvider() writes to only one. After the bump the tree holds exactly one copy, 0.220.0 (verified on disk).

  • Inherited behaviour change: scheduledDelayMillis now defaults to 1000ms, where 0.205 defaulted to 5000ms. ADK does not set the field, so batched log export becomes more frequent. This is upstream's default and I inherit it.

  • Inherited API removals: consumers who import @opentelemetry/sdk-logs themselves move to 0.220, where BufferConfig, BatchLogRecordProcessorBrowserConfig and NoopLogRecordProcessor no longer exist. ADK uses none of them. ADK's own public API is unchanged.

  • Lockfile hygiene. npm resolves versions from the abbreviated packument, which carries no license field, so the entries it rewrote lost theirs. I restored each one from that exact version's published manifest on registry.npmjs.org, and npm install then rewrites the file byte-identically. Every changed or added entry carries "license": "Apache-2.0", and the whole lockfile delta is @opentelemetry/* only.

  • Not in this PR: the @mikro-orm/sqlite 6 to 7 and esbuild 0.25 to 0.28 majors, and the concurrently patch bump, are the other three changes in Fix: honor a caller-supplied appName in the Agent Engine deploy path (and expose --app_name) #448. They are handled in separate PRs to keep one concern per review.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

I added one case to core/test/telemetry/setup_test.ts and changed no existing case. The existing cases auto-mock @opentelemetry/sdk-logs and assert only that a logger provider was registered, so they would have stayed green through this entire break. The new case asserts the exact argument object, which fails both on the old positional shape and on an unexpected extra option.

Proof the new test can fail. I restored line 163 to new BatchLogRecordProcessor(new OTLPLogExporter()) and ran npx vitest run --project unit:core core/test/telemetry/setup_test.ts. The new test failed and the six pre-existing tests all passed:

AssertionError: expected "BatchLogRecordProcessor" to be called with arguments: [ { exporter: Any<OTLPLogExporter> } ]
Received:
  1st BatchLogRecordProcessor call:
  [
-   {
-     "exporter": Any<OTLPLogExporter>,
+   OTLPLogExporter {
...
Tests  1 failed | 6 passed (7)

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

npm ci
npm run build                                          # all three workspaces succeed
npx vitest run --project unit:core core/test/telemetry/ # 20 passed
npx vitest run --project unit:dev dev/test/server/adk_api_server_test.ts  # 51 passed
npm run lint && npm run format:check && npm run docs:check               # all pass
npm audit                                              # @opentelemetry/core no longer listed

Before the fix, npm run build --workspace core fails with the TS2345 at core/src/telemetry/setup.ts:163:38.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 3 commits August 5, 2026 13:07
…ns signature

@opentelemetry/sdk-logs 0.220 replaced the BatchLogRecordProcessor
positional exporter argument with a required BatchLogRecordProcessorOptions
object, so the core build failed with TS2345 on the bump.

The four OTLP packages pin @opentelemetry/core at exactly 2.1.0 on the
0.205 line, which is why GHSA-8988-4f7v-96qf (vulnerable range < 2.8.0)
cannot be cleared without moving them. @opentelemetry/api-logs moves with
them because sdk-logs, exporter-logs-otlp-http and otlp-transformer all
pin it at exactly 0.220.0; leaving it on ^0.205.0 would install a second
copy of the logs API and a second global logger registry.
The existing cases auto-mock @opentelemetry/sdk-logs and only assert that
a logger provider was registered, so they stayed green through the 0.220
signature break. The new case asserts the exact argument object.
npm resolves versions from the abbreviated packument, which carries no
license field, so the entries it rewrote lost theirs. The values are
restored from each package's published manifest on registry.npmjs.org,
and npm rewrites the file byte-identically.
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.

1 participant