Skip to content
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.

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

feat(otlp-exporter-base): internally accept a http header provider function only #5179

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 15 additions & 12 deletions experimental/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,21 @@ All notable changes to experimental packages in this project will be documented

### :boom: Breaking Change

* feat(otlp-exporter-base)!: collapse base classes into one [#5031](https://github.com/open-telemetry/opentelemetry-js/pull/5031) @pichlermarc
* `OTLPExporterNodeBase` has been removed in favor of a platform-agnostic implementation (`OTLPExporterBase`)
* `OTLPExporterBrowserBase` has been removed in favor of a platform-agnostic implementation (`OTLPExporterBase`)
* `ExportServiceError` was intended for internal use and has been dropped from exports
* `validateAndNormalizeHeaders` was intended for internal use and has been dropped from exports
* `OTLPExporterBase` all properties are now private, the constructor now takes an `IOTLPExportDelegate`, the type parameter for config type has been dropped.
* This type is scheduled for removal in a future version of this package, please treat all exporters as `SpanExporter`, `PushMetricExporter` or `LogRecordExporter`, based on their respective type.
* feat(otlp-grpc-exporter-base)!: collapse base classes into one [#5031](https://github.com/open-telemetry/opentelemetry-js/pull/5031) @pichlermarc
* `OTLPGRPCExporterNodeBase` has been removed in favor of a platform-agnostic implementation (`OTLPExporterBase` from `@opentelemetry/otlp-exporter-base`)
Comment on lines +10 to +18
Copy link
Member Author

Choose a reason for hiding this comment

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

Note for reviewers: this was in the wrong spot earlier, so moved it up here.

* feat(otlp-exporter-base): internally accept a http header provider function only [#5179](https://github.com/open-telemetry/opentelemetry-js/pull/5179) @pichlermarc
* feat(otlp-transformer)!: accept `ResourceMetrics` in serializers instead of `ResourceMetrics[]`
* (user-facing): `ProtobufMetricsSerializer` now only accepts `ResourceMetrics` instead of `ResourceMetrics[]` to align with `PushMetricExporter` requirements
* (user-facing): `JsonMetricsSerializer` now only accepts `ResourceMetrics` instead of `ResourceMetrics[]` to align with `PushMetricExporter` requirements


### :rocket: (Enhancement)

### :bug: (Bug Fix)
Expand Down Expand Up @@ -53,18 +68,6 @@ All notable changes to experimental packages in this project will be documented
* GetFunction
* Func
* Err
* feat(otlp-exporter-base)!: collapse base classes into one [#5031](https://github.com/open-telemetry/opentelemetry-js/pull/5031)
* `OTLPExporterNodeBase` has been removed in favor of a platform-agnostic implementation (`OTLPExporterBase`)
* `OTLPExporterBrowserBase` has been removed in favor of a platform-agnostic implementation (`OTLPExporterBase`)
* `ExportServiceError` was intended for internal use and has been dropped from exports
* `validateAndNormalizeHeaders` was intended for internal use and has been dropped from exports
* `OTLPExporterBase` all properties are now private, the constructor now takes an `IOTLPExportDelegate`, the type parameter for config type has been dropped.
* This type is scheduled for removal in a future version of this package, please treat all exporters as `SpanExporter`, `PushMetricExporter` or `LogRecordExporter`, based on their respective type.
* feat(otlp-grpc-exporter-base)!: collapse base classes into one [#5031](https://github.com/open-telemetry/opentelemetry-js/pull/5031)
* `OTLPGRPCExporterNodeBase` has been removed in favor of a platform-agnostic implementation (`OTLPExporterBase` from `@opentelemetry/otlp-exporter-base`)
* feat(otlp-transformer)!: accept `ResourceMetrics` in serializers instead of `ResourceMetrics[]`
* (user-facing): `ProtobufMetricsSerializer` now only accepts `ResourceMetrics` instead of `ResourceMetrics[]` to align with `PushMetricExporter` requirements
* (user-facing): `JsonMetricsSerializer` now only accepts `ResourceMetrics` instead of `ResourceMetrics[]` to align with `PushMetricExporter` requirements

### :rocket: (Enhancement)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
OtlpHttpConfiguration,
} from './otlp-http-configuration';
import { OTLPExporterNodeConfigBase } from './legacy-node-configuration';
import { wrapStaticHeadersInFunction } from './shared-configuration';

/**
* @deprecated this will be removed in 2.0
Expand All @@ -36,7 +37,7 @@ export function convertLegacyBrowserHttpOptions(
{
url: config.url,
timeoutMillis: config.timeoutMillis,
headers: config.headers,
headers: wrapStaticHeadersInFunction(config.headers),
concurrencyLimit: config.concurrencyLimit,
},
{}, // no fallback for browser case
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { getHttpConfigurationFromEnvironment } from './otlp-http-env-configurati
import type * as http from 'http';
import type * as https from 'https';
import { diag } from '@opentelemetry/api';
import { wrapStaticHeadersInFunction } from './shared-configuration';

function convertLegacyAgentOptions(
config: OTLPExporterNodeConfigBase
Expand Down Expand Up @@ -67,7 +68,7 @@ export function convertLegacyHttpOptions(
return mergeOtlpHttpConfigurationWithDefaults(
{
url: config.url,
headers: config.headers,
headers: wrapStaticHeadersInFunction(config.headers),
concurrencyLimit: config.concurrencyLimit,
timeoutMillis: config.timeoutMillis,
compression: config.compression,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,32 +27,34 @@ import type * as https from 'https';

export interface OtlpHttpConfiguration extends OtlpSharedConfiguration {
url: string;
headers: Record<string, string>;
headers: () => Record<string, string>;

Choose a reason for hiding this comment

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

Hi @pichlermarc, I have done a similar pr about that for string or function support #5118
What do you think??

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @bozzelliandrea - I'd prefer having only one option internally (as my PR here proposes) to avoid the code becoming too complex. By only dealing with provider functions internally we can handle both static and dynamic use-cases via the same code-path.

Choose a reason for hiding this comment

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

Hi @pichlermarc, do you have any idea when the team will release the version with this commit? thanks

agentOptions: http.AgentOptions | https.AgentOptions;
}

function mergeHeaders(
userProvidedHeaders: Record<string, string> | undefined | null,
fallbackHeaders: Record<string, string> | undefined | null,
defaultHeaders: Record<string, string>
): Record<string, string> {
userProvidedHeaders: (() => Record<string, string>) | undefined | null,
fallbackHeaders: (() => Record<string, string>) | undefined | null,
defaultHeaders: () => Record<string, string>
): () => Record<string, string> {
const requiredHeaders = {
...defaultHeaders,
...defaultHeaders(),
};
const headers = {};

// add fallback ones first
if (fallbackHeaders != null) {
Object.assign(headers, fallbackHeaders);
}
return () => {
// add fallback ones first
if (fallbackHeaders != null) {
Object.assign(headers, fallbackHeaders());
}

// override with user-provided ones
if (userProvidedHeaders != null) {
Object.assign(headers, userProvidedHeaders);
}
// override with user-provided ones
if (userProvidedHeaders != null) {
Object.assign(headers, userProvidedHeaders());
}

// override required ones.
return Object.assign(headers, requiredHeaders);
// override required ones.
return Object.assign(headers, requiredHeaders);
};
}

function validateUserProvidedUrl(url: string | undefined): string | undefined {
Expand Down Expand Up @@ -107,7 +109,7 @@ export function getHttpConfigurationDefaults(
): OtlpHttpConfiguration {
return {
...getSharedConfigurationDefaults(),
headers: requiredHeaders,
headers: () => requiredHeaders,
url: 'http://localhost:4318/' + signalResourcePath,
agentOptions: { keepAlive: true },
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@ import { baggageUtils } from '@opentelemetry/core';
import { diag } from '@opentelemetry/api';
import { getSharedConfigurationFromEnvironment } from './shared-env-configuration';
import { OtlpHttpConfiguration } from './otlp-http-configuration';
import { wrapStaticHeadersInFunction } from './shared-configuration';

function getHeadersFromEnv(signalIdentifier: string) {
function getStaticHeadersFromEnv(
signalIdentifier: string
): Record<string, string> | undefined {
const signalSpecificRawHeaders =
process.env[`OTEL_EXPORTER_OTLP_${signalIdentifier}_HEADERS`]?.trim();
const nonSignalSpecificRawHeaders =
Expand Down Expand Up @@ -126,6 +129,8 @@ export function getHttpConfigurationFromEnvironment(
url:
getSpecificUrlFromEnv(signalIdentifier) ??
getNonSpecificUrlFromEnv(signalResourcePath),
headers: getHeadersFromEnv(signalIdentifier),
headers: wrapStaticHeadersInFunction(
getStaticHeadersFromEnv(signalIdentifier)
),
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,16 @@ export function validateTimeoutMillis(timeoutMillis: number) {
);
}

export function wrapStaticHeadersInFunction(
headers: Record<string, string> | undefined
): (() => Record<string, string>) | undefined {
if (headers == null) {
return undefined;
}

return () => headers;
}

/**
* @param userProvidedConfiguration Configuration options provided by the user in code.
* @param fallbackConfiguration Fallback to use when the {@link userProvidedConfiguration} does not specify an option.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export function createOtlpSendBeaconExportDelegate<Internal, Response>(
createRetryingTransport({
transport: createSendBeaconTransport({
url: options.url,
blobType: options.headers['Content-Type'],
blobType: options.headers()['Content-Type'],
}),
})
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export type sendWithHttp = (

export interface HttpRequestParameters {
url: string;
headers: Record<string, string>;
headers: () => Record<string, string>;
compression: 'gzip' | 'none';
agentOptions: http.AgentOptions | https.AgentOptions;
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ export function sendWithHttp(
path: parsedUrl.pathname,
method: 'POST',
headers: {
...params.headers,
...params.headers(),
},
agent: agent,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import {

export interface XhrRequestParameters {
url: string;
headers: Record<string, string>;
headers: () => Record<string, string>;
}

class XhrTransport implements IExporterTransport {
Expand All @@ -35,7 +35,8 @@ class XhrTransport implements IExporterTransport {
const xhr = new XMLHttpRequest();
xhr.timeout = timeoutMillis;
xhr.open('POST', this._parameters.url);
Object.entries(this._parameters.headers).forEach(([k, v]) => {
const headers = this._parameters.headers();
Object.entries(headers).forEach(([k, v]) => {
xhr.setRequestHeader(k, v);
});

Expand Down Expand Up @@ -80,9 +81,7 @@ class XhrTransport implements IExporterTransport {
});
};

xhr.send(
new Blob([data], { type: this._parameters.headers['Content-Type'] })
);
xhr.send(new Blob([data], { type: headers['Content-Type'] }));
});
}

Expand Down
28 changes: 15 additions & 13 deletions experimental/packages/otlp-exporter-base/src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,19 @@ import { diag } from '@opentelemetry/api';
* @param partialHeaders
*/
export function validateAndNormalizeHeaders(
partialHeaders: Partial<Record<string, unknown>> = {}
): Record<string, string> {
const headers: Record<string, string> = {};
Object.entries(partialHeaders).forEach(([key, value]) => {
if (typeof value !== 'undefined') {
headers[key] = String(value);
} else {
diag.warn(
`Header "${key}" has invalid value (${value}) and will be ignored`
);
}
});
return headers;
partialHeaders: (() => Record<string, string>) | undefined
): () => Record<string, string> {
return () => {
const headers: Record<string, string> = {};
Object.entries(partialHeaders?.() ?? {}).forEach(([key, value]) => {
if (typeof value !== 'undefined') {
headers[key] = String(value);
} else {
diag.warn(
`Header "${key}" has invalid value (${value}) and will be ignored`
);
}
});
return headers;
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ import { ensureHeadersContain } from '../testHelper';

const testTransportParameters = {
url: 'http://example.test',
headers: {
headers: () => ({
foo: 'foo-value',
bar: 'bar-value',
'Content-Type': 'application/json',
},
}),
};

const requestTimeout = 1000;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,22 +27,22 @@ describe('mergeOtlpHttpConfigurationWithDefaults', function () {
timeoutMillis: 1,
compression: 'none',
concurrencyLimit: 2,
headers: { 'User-Agent': 'default-user-agent' },
headers: () => ({ 'User-Agent': 'default-user-agent' }),
agentOptions: { keepAlive: true },
};

describe('headers', function () {
it('merges headers instead of overriding', function () {
const config = mergeOtlpHttpConfigurationWithDefaults(
{
headers: { foo: 'user' },
headers: () => ({ foo: 'user' }),
},
{
headers: { foo: 'fallback', bar: 'fallback' },
headers: () => ({ foo: 'fallback', bar: 'fallback' }),
},
testDefaults
);
assert.deepStrictEqual(config.headers, {
assert.deepStrictEqual(config.headers(), {
'User-Agent': 'default-user-agent',
foo: 'user',
bar: 'fallback',
Expand All @@ -52,14 +52,14 @@ describe('mergeOtlpHttpConfigurationWithDefaults', function () {
it('does not override default (required) header', function () {
const config = mergeOtlpHttpConfigurationWithDefaults(
{
headers: { 'User-Agent': 'custom' },
headers: () => ({ 'User-Agent': 'custom' }),
},
{
headers: { 'User-Agent': 'custom-fallback' },
headers: () => ({ 'User-Agent': 'custom-fallback' }),
},
testDefaults
);
assert.deepStrictEqual(config.headers, {
assert.deepStrictEqual(config.headers(), {
'User-Agent': 'default-user-agent',
});
});
Expand All @@ -69,12 +69,16 @@ describe('mergeOtlpHttpConfigurationWithDefaults', function () {
{
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore simulating plain JavaScript usage, ignoring types
headers: { foo: 'foo-user-provided', bar: undefined, baz: null },
headers: () => ({
foo: 'foo-user-provided',
bar: undefined,
baz: null,
}),
},
{},
testDefaults
);
assert.deepStrictEqual(config.headers, {
assert.deepStrictEqual(config.headers(), {
foo: 'foo-user-provided',
baz: 'null',
'User-Agent': 'default-user-agent',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ describe('parseHeaders', function () {
foo2: 'bar',
foo3: 1,
};
const result = validateAndNormalizeHeaders(headers);
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore simulating plain JS usage
const result = validateAndNormalizeHeaders(() => headers)();
assert.deepStrictEqual(result, {
foo2: 'bar',
foo3: '1',
Expand All @@ -44,7 +46,7 @@ describe('parseHeaders', function () {
});

it('should parse undefined', function () {
const result = validateAndNormalizeHeaders(undefined);
const result = validateAndNormalizeHeaders(undefined)();
assert.deepStrictEqual(result, {});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ describe('getHttpConfigurationFromEnvironment', function () {
'METRICS',
'v1/metrics'
);
assert.deepEqual(config.headers, {
assert.deepEqual(config.headers?.(), {
key1: 'metrics',
key2: 'value2',
});
Expand All @@ -62,7 +62,7 @@ describe('getHttpConfigurationFromEnvironment', function () {
'METRICS',
'v1/metrics'
);
assert.deepEqual(config.headers, {
assert.deepEqual(config.headers?.(), {
key1: 'value1',
key2: 'value2',
});
Expand All @@ -76,7 +76,7 @@ describe('getHttpConfigurationFromEnvironment', function () {
'METRICS',
'v1/metrics'
);
assert.deepEqual(config.headers, {
assert.deepEqual(config.headers?.(), {
key1: 'value1',
key2: 'value2',
});
Expand Down
Loading
Loading