Skip to content

feat(collector-exporter): allow configuration from env #2099 #2101

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

Closed
vmarchaud wants to merge 1 commit into from
Closed

feat(collector-exporter): allow configuration from env #2099 #2101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
66 changes: 12 additions & 54 deletions packages/opentelemetry-core/src/baggage/propagation/HttpBaggage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/

import {
Baggage,
Context,
BaggageEntry,
getBaggage,
Expand All @@ -24,22 +23,17 @@ import {
TextMapPropagator,
TextMapSetter,
createBaggage,
baggageEntryMetadataFromString,
isInstrumentationSuppressed,
} from '@opentelemetry/api';

const KEY_PAIR_SEPARATOR = '=';
const PROPERTIES_SEPARATOR = ';';
const ITEMS_SEPARATOR = ',';

// Name of the http header used to propagate the baggage
export const BAGGAGE_HEADER = 'baggage';
// Maximum number of name-value pairs allowed by w3c spec
export const MAX_NAME_VALUE_PAIRS = 180;
// Maximum number of bytes per a single name-value pair allowed by w3c spec
export const MAX_PER_NAME_VALUE_PAIRS = 4096;
// Maximum total length of all name-value pairs allowed by w3c spec
export const MAX_TOTAL_LENGTH = 8192;
import {
getKeyPairs,
serializeKeyPairs,
parsePairKeyValue,
MAX_NAME_VALUE_PAIRS,
ITEMS_SEPARATOR,
BAGGAGE_HEADER,
MAX_PER_NAME_VALUE_PAIRS,
} from '../utils';

/**
* Propagates {@link Baggage} through Context format propagation.
Expand All @@ -51,35 +45,17 @@ export class HttpBaggage implements TextMapPropagator {
inject(context: Context, carrier: unknown, setter: TextMapSetter) {
const baggage = getBaggage(context);
if (!baggage || isInstrumentationSuppressed(context)) return;
const keyPairs = this._getKeyPairs(baggage)
const keyPairs = getKeyPairs(baggage)
.filter((pair: string) => {
return pair.length <= MAX_PER_NAME_VALUE_PAIRS;
})
.slice(0, MAX_NAME_VALUE_PAIRS);
const headerValue = this._serializeKeyPairs(keyPairs);
const headerValue = serializeKeyPairs(keyPairs);
if (headerValue.length > 0) {
setter.set(carrier, BAGGAGE_HEADER, headerValue);
}
}

private _serializeKeyPairs(keyPairs: string[]) {
return keyPairs.reduce((hValue: string, current: string) => {
const value = `${hValue}${
hValue !== '' ? ITEMS_SEPARATOR : ''
}${current}`;
return value.length > MAX_TOTAL_LENGTH ? hValue : value;
}, '');
}

private _getKeyPairs(baggage: Baggage): string[] {
return baggage
.getAllEntries()
.map(
([key, value]) =>
`${encodeURIComponent(key)}=${encodeURIComponent(value.value)}`
);
}

extract(context: Context, carrier: unknown, getter: TextMapGetter): Context {
const headerValue: string = getter.get(carrier, BAGGAGE_HEADER) as string;
if (!headerValue) return context;
Expand All @@ -89,7 +65,7 @@ export class HttpBaggage implements TextMapPropagator {
}
const pairs = headerValue.split(ITEMS_SEPARATOR);
pairs.forEach(entry => {
const keyPair = this._parsePairKeyValue(entry);
const keyPair = parsePairKeyValue(entry);
if (keyPair) {
const baggageEntry: BaggageEntry = { value: keyPair.value };
if (keyPair.metadata) {
Expand All @@ -104,24 +80,6 @@ export class HttpBaggage implements TextMapPropagator {
return setBaggage(context, createBaggage(baggage));
}

private _parsePairKeyValue(entry: string) {
const valueProps = entry.split(PROPERTIES_SEPARATOR);
if (valueProps.length <= 0) return;
const keyPairPart = valueProps.shift();
if (!keyPairPart) return;
const keyPair = keyPairPart.split(KEY_PAIR_SEPARATOR);
if (keyPair.length !== 2) return;
const key = decodeURIComponent(keyPair[0].trim());
const value = decodeURIComponent(keyPair[1].trim());
let metadata;
if (valueProps.length > 0) {
metadata = baggageEntryMetadataFromString(
valueProps.join(PROPERTIES_SEPARATOR)
);
}
return { key, value, metadata };
}

fields(): string[] {
return [BAGGAGE_HEADER];
}
Expand Down
63 changes: 63 additions & 0 deletions packages/opentelemetry-core/src/baggage/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import { Baggage, baggageEntryMetadataFromString } from '@opentelemetry/api';

export const KEY_PAIR_SEPARATOR = '=';
Copy link
Member

@obecny obecny Apr 14, 2021

Choose a reason for hiding this comment

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

if you split this already what if you keep all const in separate file ? constants.ts for example ?
and then later in code instead of

baggageUtils.BAGGAGE_HEADER

you will have for example

baggageConst.BAGGAGE_HEADER
// or 
constants.BAGGAGE_HEADER

export const PROPERTIES_SEPARATOR = ';';
export const ITEMS_SEPARATOR = ',';

// Name of the http header used to propagate the baggage
export const BAGGAGE_HEADER = 'baggage';
// Maximum number of name-value pairs allowed by w3c spec
export const MAX_NAME_VALUE_PAIRS = 180;
// Maximum number of bytes per a single name-value pair allowed by w3c spec
export const MAX_PER_NAME_VALUE_PAIRS = 4096;
// Maximum total length of all name-value pairs allowed by w3c spec
export const MAX_TOTAL_LENGTH = 8192;

export const serializeKeyPairs = (keyPairs: string[]) => {
return keyPairs.reduce((hValue: string, current: string) => {
const value = `${hValue}${hValue !== '' ? ITEMS_SEPARATOR : ''}${current}`;
return value.length > MAX_TOTAL_LENGTH ? hValue : value;
}, '');
};

export const getKeyPairs = (baggage: Baggage): string[] => {
return baggage
.getAllEntries()
.map(
([key, value]) =>
`${encodeURIComponent(key)}=${encodeURIComponent(value.value)}`
);
};

export const parsePairKeyValue = (entry: string) => {
const valueProps = entry.split(PROPERTIES_SEPARATOR);
if (valueProps.length <= 0) return;
Copy link
Member

@obecny obecny Apr 14, 2021

Choose a reason for hiding this comment

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

Nit / suggestions

Suggested change
if (valueProps.length <= 0) return;
if (valueProps.length <= 0) {
return;
}

const keyPairPart = valueProps.shift();
if (!keyPairPart) return;
const keyPair = keyPairPart.split(KEY_PAIR_SEPARATOR);
if (keyPair.length !== 2) return;
const key = decodeURIComponent(keyPair[0].trim());
Copy link
Member

@obecny obecny Apr 14, 2021

Choose a reason for hiding this comment

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

Nit: line space

const value = decodeURIComponent(keyPair[1].trim());
let metadata;
if (valueProps.length > 0) {
metadata = baggageEntryMetadataFromString(
valueProps.join(PROPERTIES_SEPARATOR)
);
}
return { key, value, metadata };
};
1 change: 1 addition & 0 deletions packages/opentelemetry-core/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export * from './context/propagation/composite';
export * from './context/propagation/HttpTraceContext';
export * from './context/propagation/types';
export * from './baggage/propagation/HttpBaggage';
export * as baggageUtils from './baggage/utils';
export * from './platform';
export * from './trace/sampler/AlwaysOffSampler';
export * from './trace/sampler/AlwaysOnSampler';
Expand Down
12 changes: 12 additions & 0 deletions packages/opentelemetry-core/src/utils/environment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ export type ENVIRONMENT = {
OTEL_EXPORTER_JAEGER_USER?: string;
OTEL_LOG_LEVEL?: DiagLogLevel;
OTEL_RESOURCE_ATTRIBUTES?: string;
OTEL_EXPORTER_OTLP_ENDPOINT?: string;
Copy link
Member

@obecny obecny Apr 14, 2021

Choose a reason for hiding this comment

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

pls keep it in alphabetical order so it is easier to find / add new one

OTEL_EXPORTER_OTLP_TRACES_ENDPOINT?: string;
OTEL_EXPORTER_OTLP_METRICS_ENDPOINT?: string;
OTEL_EXPORTER_OTLP_HEADERS?: string;
OTEL_EXPORTER_OTLP_TRACES_HEADERS?: string;
OTEL_EXPORTER_OTLP_METRICS_HEADERS?: string;
} & ENVIRONMENT_NUMBERS &
ENVIRONMENT_LISTS;

Expand Down Expand Up @@ -102,6 +108,12 @@ export const DEFAULT_ENVIRONMENT: Required<ENVIRONMENT> = {
OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT: 1000,
OTEL_SPAN_EVENT_COUNT_LIMIT: 1000,
OTEL_SPAN_LINK_COUNT_LIMIT: 1000,
OTEL_EXPORTER_OTLP_ENDPOINT: '',
Copy link
Member

@obecny obecny Apr 14, 2021

Choose a reason for hiding this comment

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

pls keep it in alphabetical order so it is easier to find / add new one

OTEL_EXPORTER_OTLP_TRACES_ENDPOINT: '',
OTEL_EXPORTER_OTLP_METRICS_ENDPOINT: '',
OTEL_EXPORTER_OTLP_HEADERS: '',
OTEL_EXPORTER_OTLP_TRACES_HEADERS: '',
OTEL_EXPORTER_OTLP_METRICS_HEADERS: '',
};

/**
Expand Down
30 changes: 15 additions & 15 deletions packages/opentelemetry-core/test/baggage/HttpBaggage.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,7 @@ import {
} from '@opentelemetry/api';
import { ROOT_CONTEXT } from '@opentelemetry/api';
import * as assert from 'assert';
import {
BAGGAGE_HEADER,
HttpBaggage,
} from '../../src/baggage/propagation/HttpBaggage';
import { baggageUtils, HttpBaggage } from '../../src/';

describe('HttpBaggage', () => {
const httpTraceContext = new HttpBaggage();
Expand All @@ -53,7 +50,7 @@ describe('HttpBaggage', () => {
defaultTextMapSetter
);
assert.deepStrictEqual(
carrier[BAGGAGE_HEADER],
carrier[baggageUtils.BAGGAGE_HEADER],
'key1=d4cda95b652f4a1592b449d5929fda1b,key3=c88815a7-0fa9-4d95-a1f1-cdccce3c5c2a,with%2Fslash=with%20spaces'
);
});
Expand All @@ -70,7 +67,7 @@ describe('HttpBaggage', () => {
defaultTextMapSetter
);
assert.deepStrictEqual(
carrier[BAGGAGE_HEADER],
carrier[baggageUtils.BAGGAGE_HEADER],
'key1=d4cda95b,key3=c88815a7'
);
});
Expand All @@ -91,7 +88,7 @@ describe('HttpBaggage', () => {
defaultTextMapSetter
);

let header = carrier[BAGGAGE_HEADER];
let header = carrier[baggageUtils.BAGGAGE_HEADER];
assert.ok(typeof header === 'string');
assert.deepStrictEqual(header, `aa=shortvalue,${shortKey}=${value}`);

Expand All @@ -107,7 +104,7 @@ describe('HttpBaggage', () => {
defaultTextMapSetter
);

header = carrier[BAGGAGE_HEADER];
header = carrier[baggageUtils.BAGGAGE_HEADER];
assert.ok(typeof header === 'string');
assert.deepStrictEqual(header, 'aa=shortvalue');
});
Expand All @@ -129,7 +126,7 @@ describe('HttpBaggage', () => {
defaultTextMapSetter
);

let header = carrier[BAGGAGE_HEADER];
let header = carrier[baggageUtils.BAGGAGE_HEADER];
assert.ok(typeof header === 'string');
assert.deepStrictEqual(header.length, 8192);
assert.deepStrictEqual(header.split(',').length, 3);
Expand All @@ -147,7 +144,7 @@ describe('HttpBaggage', () => {
defaultTextMapSetter
);

header = carrier[BAGGAGE_HEADER];
header = carrier[baggageUtils.BAGGAGE_HEADER];
assert.ok(typeof header === 'string');
assert.deepStrictEqual(header.length, 8100);
assert.deepStrictEqual(header.split(',').length, 2);
Expand All @@ -170,15 +167,15 @@ describe('HttpBaggage', () => {
defaultTextMapSetter
);

const header = carrier[BAGGAGE_HEADER];
const header = carrier[baggageUtils.BAGGAGE_HEADER];
assert.ok(typeof header === 'string');
assert.strictEqual(header.split(',').length, 180);
});
});

describe('.extract()', () => {
it('should extract context of a sampled span from carrier', () => {
carrier[BAGGAGE_HEADER] =
carrier[baggageUtils.BAGGAGE_HEADER] =
'key1=d4cda95b,key3=c88815a7, keyn = valn, keym =valm';
const extractedBaggage = getBaggage(
httpTraceContext.extract(ROOT_CONTEXT, carrier, defaultTextMapGetter)
Expand All @@ -197,7 +194,9 @@ describe('HttpBaggage', () => {
describe('fields()', () => {
it('returns the fields used by the baggage spec', () => {
const propagator = new HttpBaggage();
assert.deepStrictEqual(propagator.fields(), [BAGGAGE_HEADER]);
assert.deepStrictEqual(propagator.fields(), [
baggageUtils.BAGGAGE_HEADER,
]);
});
});

Expand All @@ -211,7 +210,8 @@ describe('HttpBaggage', () => {
});

it('returns keys with their properties', () => {
carrier[BAGGAGE_HEADER] = 'key1=d4cda95b,key3=c88815a7;prop1=value1';
carrier[baggageUtils.BAGGAGE_HEADER] =
'key1=d4cda95b,key3=c88815a7;prop1=value1';
const bag = getBaggage(
httpTraceContext.extract(ROOT_CONTEXT, carrier, defaultTextMapGetter)
);
Expand Down Expand Up @@ -262,7 +262,7 @@ describe('HttpBaggage', () => {
},
};
Object.getOwnPropertyNames(testCases).forEach(testCase => {
carrier[BAGGAGE_HEADER] = testCases[testCase].header;
carrier[baggageUtils.BAGGAGE_HEADER] = testCases[testCase].header;

const extractedSpanContext = getBaggage(
httpTraceContext.extract(ROOT_CONTEXT, carrier, defaultTextMapGetter)
Expand Down
7 changes: 1 addition & 6 deletions packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,7 @@ import {
collectorTypes,
} from '@opentelemetry/exporter-collector';
import type { Metadata } from 'grpc';
import {
CollectorExporterConfigNode,
GRPCQueueItem,
ServiceClientType,
} from './types';
import { CollectorExporterConfigNode, GRPCQueueItem } from './types';
import { ServiceClient } from './types';

/**
Expand Down Expand Up @@ -119,5 +115,4 @@ export abstract class CollectorExporterNodeBase<
}

abstract getServiceProtoPath(): string;
abstract getServiceClientType(): ServiceClientType;
}
23 changes: 6 additions & 17 deletions packages/opentelemetry-exporter-collector-grpc/src/CollectorMetricExporter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,8 @@ import {
toCollectorExportMetricServiceRequest,
} from '@opentelemetry/exporter-collector';
import { MetricRecord, MetricExporter } from '@opentelemetry/metrics';
import { CollectorExporterConfigNode, ServiceClientType } from './types';
import { CollectorExporterNodeBase } from './CollectorExporterNodeBase';

const DEFAULT_SERVICE_NAME = 'collector-metric-exporter';
const DEFAULT_COLLECTOR_URL = 'localhost:4317';

/**
* Collector Metric Exporter for Node
*/
Expand All @@ -47,22 +43,15 @@ export class CollectorMetricExporter
);
}

getDefaultUrl(config: CollectorExporterConfigNode): string {
if (!config.url) {
return DEFAULT_COLLECTOR_URL;
}
return config.url;
}

getDefaultServiceName(config: CollectorExporterConfigNode): string {
return config.serviceName || DEFAULT_SERVICE_NAME;
getServiceProtoPath(): string {
return 'opentelemetry/proto/collector/metrics/v1/metrics_service.proto';
}

getServiceClientType() {
Copy link
Member

@obecny obecny Apr 14, 2021

Choose a reason for hiding this comment

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

hmm not sure why was this removed, it will be still needed to differentiate between metrics and traces - it is metrics and you changed this to be traces ?

return ServiceClientType.METRICS;
getExporterType() {
return 'trace' as const;
}

getServiceProtoPath(): string {
Copy link
Member

@obecny obecny Apr 14, 2021

Choose a reason for hiding this comment

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

this is also needed to correctly import the appropriate proto definitions.
Is this on purpose and if so how will the class know which proto definition to load ? - different for metrics and different for traces

return 'opentelemetry/proto/collector/metrics/v1/metrics_service.proto';
getProtocol() {
return 'grpc' as const;
}
}
Loading