Skip to content

refactor!(otlp-grpc-exporter-base): remove headers from gRPC config type #6487

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dyladan merged 7 commits into from
Mar 11, 2026
Merged

refactor!(otlp-grpc-exporter-base): remove headers from gRPC config type #6487

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
05de16b
fix(otlp-grpc-exporter-base): remove headers from gRPC config type
pratik50 Mar 10, 2026
a9356c7
chore: fix lint and update CHANGELOG
pratik50 Mar 10, 2026
f45ce74
Merge remote-tracking branch 'upstream/main' into fix/grpc-exporter-r…
pratik50 Mar 10, 2026
d462ed7
resolve merge conflict with upstream/main
pratik50 Mar 10, 2026
6b10bfa
fix CHANGELOG entries and proper package-lock.json
pratik50 Mar 10, 2026
dff2829
refactor(otlp-grpc-exporter-base): use Omit to exclude headers from g…
pratik50 Mar 11, 2026
6241597
chore: restore accidentally deleted CHANGELOG entry
pratik50 Mar 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ For notes on migrating to 2.x / 0.200.x see [the upgrade guide](doc/upgrade-to-2

* fix(opentelemetry-instrumentation): improve `_warnOnPreloadedModules` function not to show warning logs when the module is not marked as loaded [#6095](https://github.com/open-telemetry/opentelemetry-js/pull/6095) @rlj1202
* fix(sdk-trace-base): derive internal `SpanOptions` from API type to prevent drift [#6478](https://github.com/open-telemetry/opentelemetry-js/pull/6478) @overbalance
* fix(span): enforce `attributePerEventCountLimit`, `attributePerLinkCountLimit`, `linkCountLimit`, and `attributeValueLengthLimit` for event/link attributes [#6479](https://github.com/open-telemetry/opentelemetry-js/pull/6479) @overbalance

@dyladan dyladan Mar 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Revert this unintended change

* fix(otlp-grpc-exporter-base): remove `headers` from gRPC exporter config type, passing headers now results in a compile-time error instead of being silently ignored [#6487](https://github.com/open-telemetry/opentelemetry-js/pull/6487)

@dyladan dyladan Mar 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is in the wrong place and replaces an existing entry rather than creating a new one. This should be under the :breaking: header in experimental/CHANGELOG.md. It is also not a bugfix so i'd consider switching this to be a refactor! type.


### :books: Documentation

Expand Down
5 changes: 0 additions & 5 deletions ...al/packages/otlp-grpc-exporter-base/src/configuration/convert-legacy-otlp-grpc-options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
* SPDX-License-Identifier: Apache-2.0
*/
import type { OTLPGRPCExporterConfigNode } from '../types';
import { diag } from '@opentelemetry/api';
import type { OtlpGrpcConfiguration } from './otlp-grpc-configuration';
import {
getOtlpGrpcDefaultConfiguration,
Expand All @@ -21,10 +20,6 @@ export function convertLegacyOtlpGrpcOptions(
config: OTLPGRPCExporterConfigNode,
signalIdentifier: string
): OtlpGrpcConfiguration {
if (config.headers) {
diag.warn('Headers cannot be set when using grpc');
}

// keep credentials locally in case user updates the reference on the config object
const userProvidedCredentials = config.credentials;
return mergeOtlpGrpcConfigurationWithDefaults(
Expand Down
11 changes: 5 additions & 6 deletions experimental/packages/otlp-grpc-exporter-base/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,15 @@
*/

import type { ChannelCredentials, Metadata } from '@grpc/grpc-js';

import type {
CompressionAlgorithm,
OTLPExporterConfigBase,
} from '@opentelemetry/otlp-exporter-base';
import type { CompressionAlgorithm } from '@opentelemetry/otlp-exporter-base';

/**
* OTLP Exporter Config for Node
*/
export interface OTLPGRPCExporterConfigNode extends OTLPExporterConfigBase {
export interface OTLPGRPCExporterConfigNode {
url?: string;
concurrencyLimit?: number;
timeoutMillis?: number;

@dyladan dyladan Mar 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These typically come from OTLPExporterConfigBase. If they're removed here we risk them becoming out of sync. It would be better to remove the bad header property from the base and add it specifically to the HTTP exporters. Alternatively you could use Omit<T, K> to remove it from the parent class before using it here.

@pratik50 pratik50 Mar 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Switched to Omit<OTLPExporterConfigBase, 'headers'> and All tests passed. Pls review it.

@pichlermarc pichlermarc Mar 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hmm, IMO it's a positive thing that they diverge:

  • they're different exporters and the same options (url in particular) behave differently based on whether they're used for gRPC or http, which makes them hard to document.
  • we also don't consume OTLPGRPCExporterConfigNode as OTLPExporterConfigBase anywhere anymore.
  • it makes it less likely that things are added to the base that unintentionally get added to the gRPC exporter's public API which can confuse users (like what happened to headers in this case).

@pratik50 pratik50 Mar 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That makes sense! So should I revert back to the original approach (manually listing the fields with TS types) and drop the Omit change?

@pichlermarc pichlermarc Mar 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can do that in a follow up PR, your current change addresses what the linked issue was about. :)

@pratik50 pratik50 Mar 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure!

credentials?: ChannelCredentials;
metadata?: Metadata;
compression?: CompressionAlgorithm;
Expand Down
13 changes: 1 addition & 12 deletions package-lock.json
View file
Open in desktop

@dyladan dyladan Mar 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unrelated change

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.