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: add the ability to set the views via the SDK constructor #3124

Merged
merged 24 commits into from
Aug 17, 2022
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
73618d3
feat: add the ability to set the views via the SDK constructor
tapico-weyert Jul 28, 2022
65e7e79
docs: update CHANGELOG.md
tapico-weyert Jul 28, 2022
ddf099c
Merge branch 'main' into add-views-to-node-sdk
weyert Jul 28, 2022
8784954
test: added test for checking if views configured when passed to the SDK
tapico-weyert Jul 31, 2022
fa73a72
Merge branch 'main' into add-views-to-node-sdk
weyert Jul 31, 2022
ef74b98
Merge branch 'main' into add-views-to-node-sdk
weyert Aug 4, 2022
82e4c1f
test: update test to check view got applied by renaming an counter me…
tapico-weyert Aug 4, 2022
2c61e53
style: run `lint:fix` command on package
tapico-weyert Aug 5, 2022
696fa99
style: remove unused imports
tapico-weyert Aug 5, 2022
2f0b3d3
style: fix identation linting issue in `sdk.test.ts`
tapico-weyert Aug 5, 2022
9e81297
docs: add reference to views-parameter of the NodeSDK
tapico-weyert Aug 5, 2022
e278828
Merge branch 'main' into add-views-to-node-sdk
weyert Aug 5, 2022
f0d66eb
fix: throw an error when NodeSDK is used incorrectly
tapico-weyert Aug 5, 2022
620a71c
Merge branch 'main' into add-views-to-node-sdk
weyert Aug 6, 2022
635de6d
Merge branch 'main' into add-views-to-node-sdk
weyert Aug 7, 2022
8ed5a43
Merge branch 'main' into add-views-to-node-sdk
weyert Aug 9, 2022
6d52622
fix: improve the way handling views without metric reader
tapico-weyert Aug 10, 2022
eab49f7
Merge branch 'main' into add-views-to-node-sdk
weyert Aug 10, 2022
3a14a83
Merge branch 'main' into add-views-to-node-sdk
weyert Aug 13, 2022
b7412aa
Merge branch 'main' into add-views-to-node-sdk
weyert Aug 15, 2022
798aee4
chore: update code
tapico-weyert Aug 15, 2022
57af7b2
test: update the test cases
tapico-weyert Aug 15, 2022
c55f510
fix: update reader, if needed
tapico-weyert Aug 15, 2022
2931a15
Merge branch 'main' into add-views-to-node-sdk
weyert Aug 16, 2022
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
1 change: 1 addition & 0 deletions experimental/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ All notable changes to experimental packages in this project will be documented

### :rocket: (Enhancement)

* fix(add-views-to-node-sdk): added the ability to define meter views in `NodeSDK` [#3066](https://github.com/open-telemetry/opentelemetry-js/pull/3124) @weyert
* feature(prometheus-serialiser): export the unit block when unit is set in metric descriptor [#3066](https://github.com/open-telemetry/opentelemetry-js/pull/3041) @weyert

### :bug: (Bug Fix)
Expand Down
4 changes: 4 additions & 0 deletions experimental/packages/opentelemetry-sdk-node/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,10 @@ Configure a trace exporter. If an exporter OR span processor is not configured,

Configure tracing parameters. These are the same trace parameters used to [configure a tracer](../../../packages/opentelemetry-sdk-trace-base/src/types.ts#L71).

### views

Configure views of your instruments and accepts an array of [View](../opentelemetry-sdk-metrics-base/src/view/View.ts)-instances. The parameter can be used to configure the explicit bucket sizes of histogram metrics.

### serviceName

Configure the [service name](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/semantic_conventions/README.md#service).
Expand Down
39 changes: 28 additions & 11 deletions experimental/packages/opentelemetry-sdk-node/src/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
Resource,
ResourceDetectionConfig
} from '@opentelemetry/resources';
import { MeterProvider, MetricReader } from '@opentelemetry/sdk-metrics-base';
import { MeterProvider, MetricReader, View } from '@opentelemetry/sdk-metrics-base';
import {
BatchSpanProcessor,
SpanProcessor
Expand All @@ -37,15 +37,29 @@ import { SemanticResourceAttributes } from '@opentelemetry/semantic-conventions'
import { NodeSDKConfiguration } from './types';

/** This class represents everything needed to register a fully configured OpenTelemetry Node.js SDK */

export type MeterProviderConfig = {
/**
* Reference to the MetricReader instance by the NodeSDK
*/
reader?: MetricReader
/**
* Lists the views that should be passed when meterProvider
*
* Note: This is only getting used when NodeSDK is responsible for
* instantiated an instance of MeterProvider
*/
views?: View[]
};
export class NodeSDK {
private _tracerProviderConfig?: {
tracerConfig: NodeTracerConfig;
spanProcessor: SpanProcessor;
contextManager?: ContextManager;
textMapPropagator?: TextMapPropagator;
};
private _meterProviderConfig?: MeterProviderConfig;
private _instrumentations: InstrumentationOption[];
private _metricReader?: MetricReader;

private _resource: Resource;

Expand Down Expand Up @@ -87,9 +101,7 @@ export class NodeSDK {
);
}

if (configuration.metricReader) {
this.configureMeterProvider(configuration.metricReader);
}
this.configureMeterProvider({ reader: configuration.metricReader, views: configuration.views });

let instrumentations: InstrumentationOption[] = [];
if (configuration.instrumentations) {
Expand All @@ -114,16 +126,20 @@ export class NodeSDK {
}

/** Set configurations needed to register a MeterProvider */
public configureMeterProvider(reader: MetricReader): void {
this._metricReader = reader;
public configureMeterProvider(config: MeterProviderConfig): void {
if (config?.views && !config.reader) {
throw new Error('A list of views have been passed but the NodeSDK expects that the MeterProvider is manually instantiated and can\'t attach Views to the MeterProvider');
weyert marked this conversation as resolved.
Show resolved Hide resolved
}

this._meterProviderConfig = config;
}

/** Detect resource attributes */
public async detectResources(
config?: ResourceDetectionConfig
): Promise<void> {
const internalConfig: ResourceDetectionConfig = {
detectors: [ envDetector, processDetector],
detectors: [envDetector, processDetector],
...config,
};

Expand All @@ -146,7 +162,7 @@ export class NodeSDK {
this._resource = this._serviceName === undefined
? this._resource
: this._resource.merge(new Resource(
{[SemanticResourceAttributes.SERVICE_NAME]: this._serviceName}
{ [SemanticResourceAttributes.SERVICE_NAME]: this._serviceName }
));

if (this._tracerProviderConfig) {
Expand All @@ -164,12 +180,13 @@ export class NodeSDK {
});
}

if (this._metricReader) {
if (this._meterProviderConfig?.reader) {
weyert marked this conversation as resolved.
Show resolved Hide resolved
const meterProvider = new MeterProvider({
resource: this._resource,
views: this._meterProviderConfig?.views ?? [],
});

meterProvider.addMetricReader(this._metricReader);
meterProvider.addMetricReader(this._meterProviderConfig.reader);

this._meterProvider = meterProvider;

Expand Down
3 changes: 2 additions & 1 deletion experimental/packages/opentelemetry-sdk-node/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import type { ContextManager, SpanAttributes } from '@opentelemetry/api';
import { TextMapPropagator } from '@opentelemetry/api';
import { InstrumentationOption } from '@opentelemetry/instrumentation';
import { Resource } from '@opentelemetry/resources';
import { MetricReader } from '@opentelemetry/sdk-metrics-base';
import { MetricReader, View } from '@opentelemetry/sdk-metrics-base';
import {
Sampler,
SpanExporter,
Expand All @@ -32,6 +32,7 @@ export interface NodeSDKConfiguration {
defaultAttributes: SpanAttributes;
textMapPropagator: TextMapPropagator;
metricReader: MetricReader;
views: View[]
instrumentations: InstrumentationOption[];
resource: Resource;
sampler: Sampler;
Expand Down
90 changes: 85 additions & 5 deletions experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import {
AsyncLocalStorageContextManager,
} from '@opentelemetry/context-async-hooks';
import { CompositePropagator } from '@opentelemetry/core';
import { ConsoleMetricExporter, MeterProvider, PeriodicExportingMetricReader } from '@opentelemetry/sdk-metrics-base';
import { AggregationTemporality, ConsoleMetricExporter, InMemoryMetricExporter, InstrumentType, MeterProvider, PeriodicExportingMetricReader, View } from '@opentelemetry/sdk-metrics-base';
import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node';
import {
assertServiceResource,
Expand Down Expand Up @@ -147,6 +147,86 @@ describe('Node SDK', () => {
});
});

it('should register meter views when provided', async () => {
async function waitForNumberOfMetrics(exporter: InMemoryMetricExporter, numberOfMetrics: number): Promise<void> {
if (numberOfMetrics <= 0) {
throw new Error('numberOfMetrics must be greater than or equal to 0');
}

let totalExports = 0;
while (totalExports < numberOfMetrics) {
await new Promise(resolve => setTimeout(resolve, 20));
const exportedMetrics = exporter.getMetrics();
totalExports = exportedMetrics.length;
}
}

const exporter = new InMemoryMetricExporter(AggregationTemporality.CUMULATIVE);
const metricReader = new PeriodicExportingMetricReader({
exporter: exporter,
exportIntervalMillis: 100,
exportTimeoutMillis: 100
});

const sdk = new NodeSDK({
metricReader: metricReader,
views: [
new View({
name: 'test-view',
instrumentName: 'test_counter',
instrumentType: InstrumentType.COUNTER,
})
],
autoDetectResources: false,
});

await sdk.start();

assert.strictEqual(context['_getContextManager'](), ctxManager, 'context manager should not change');
assert.strictEqual(propagation['_getGlobalPropagator'](), propagator, 'propagator should not change');
assert.strictEqual((trace.getTracerProvider() as ProxyTracerProvider).getDelegate(), delegate, 'tracer provider should not have changed');

const meterProvider = metrics.getMeterProvider() as MeterProvider;
assert.ok(meterProvider);

const meter = meterProvider.getMeter('NodeSDKViews', '1.0.0');
const counter = meter.createCounter('test_counter', {
description: 'a test description',
});
counter.add(10);

await waitForNumberOfMetrics(exporter, 1);
const exportedMetrics = exporter.getMetrics();
const [firstExportedMetric] = exportedMetrics;
assert.ok(firstExportedMetric, 'should have one exported metric');
const [firstScopeMetric] = firstExportedMetric.scopeMetrics;
assert.ok(firstScopeMetric, 'should have one scope metric');
assert.ok(firstScopeMetric.scope.name === 'NodeSDKViews', 'scope should match created view');
assert.ok(firstScopeMetric.metrics.length > 0, 'should have at least one metrics entry');
const [firstMetricRecord] = firstScopeMetric.metrics;
assert.ok(firstMetricRecord.descriptor.name === 'test-view', 'should have renamed counter metric');

await sdk.shutdown();
});

it('should warn user when meter views are provided but not a MetricReader', async () => {
assert.throws(
() => {
new NodeSDK({
views: [
new View({
name: 'test-view',
instrumentName: 'test_counter',
instrumentType: InstrumentType.COUNTER,
})
],
autoDetectResources: false,
});
}, (error: Error) => {
return error.message === 'A list of views have been passed but the NodeSDK expects that the MeterProvider is manually instantiated and can\'t attach Views to the MeterProvider';
});
});

describe('detectResources', async () => {
beforeEach(() => {
process.env.OTEL_RESOURCE_ATTRIBUTES =
Expand All @@ -163,12 +243,12 @@ describe('Node SDK', () => {
autoDetectResources: true,
});
await sdk.detectResources({
detectors: [ processDetector, {
detectors: [processDetector, {
detect() {
throw new Error('Buggy detector');
}
},
envDetector ]
envDetector]
});
const resource = sdk['_resource'];

Expand Down Expand Up @@ -277,7 +357,7 @@ describe('Node SDK', () => {
});

it('should configure service name via OTEL_SERVICE_NAME env var', async () => {
process.env.OTEL_SERVICE_NAME='env-set-name';
process.env.OTEL_SERVICE_NAME = 'env-set-name';
const sdk = new NodeSDK();

await sdk.start();
Expand All @@ -290,7 +370,7 @@ describe('Node SDK', () => {
});

it('should favor config set service name over OTEL_SERVICE_NAME env set service name', async () => {
process.env.OTEL_SERVICE_NAME='env-set-name';
process.env.OTEL_SERVICE_NAME = 'env-set-name';
const sdk = new NodeSDK({
serviceName: 'config-set-name',
});
Expand Down