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(node-sdk): added resourceDetectors option to NodeSDK #3210 #3212

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
6 changes: 6 additions & 0 deletions experimental/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ All notable changes to experimental packages in this project will be documented

### :boom: Breaking Change

* Add `resourceDetectors` option to `NodeSDK` [#3210](https://github.com/open-telemetry/opentelemetry-js/issues/3210)
* `NodeSDK.detectResources()` function is no longer able to receive config as a parameter.
Instead, the detectors are passed to the constructor.

* chore(metrics-sdk): clean up exports [#3197](https://github.com/open-telemetry/opentelemetry-js/pull/3197) @pichlermarc
* removes export for:
* `AccumulationRecord`
Expand All @@ -18,6 +22,8 @@ All notable changes to experimental packages in this project will be documented

### :rocket: (Enhancement)

* Add `resourceDetectors` option to `NodeSDK` [#3210](https://github.com/open-telemetry/opentelemetry-js/issues/3210)

### :bug: (Bug Fix)

### :books: (Refine Doc)
Expand Down
7 changes: 6 additions & 1 deletion experimental/packages/opentelemetry-sdk-node/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,14 @@ or configure each instrumentation individually.

Configure a resource. Resources may also be detected by using the `autoDetectResources` method of the SDK.

### resourceDetectors

Configure resource detectors. By default, the resource detectors are [envDetector, processDetector].
NOTE: In order to enable the detection, the parameter `autoDetectResources` has to be `true`.

### sampler

Configure a custom sampler. By default all traces will be sampled.
Configure a custom sampler. By default, all traces will be sampled.

### spanProcessor

Expand Down
10 changes: 5 additions & 5 deletions experimental/packages/opentelemetry-sdk-node/src/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
registerInstrumentations
} from '@opentelemetry/instrumentation';
import {
Detector,
detectResources,
envDetector,
processDetector,
Expand Down Expand Up @@ -62,6 +63,7 @@ export class NodeSDK {
private _instrumentations: InstrumentationOption[];

private _resource: Resource;
private _resourceDetectors: Detector[];

private _autoDetectResources: boolean;

Expand All @@ -74,6 +76,7 @@ export class NodeSDK {
*/
public constructor(configuration: Partial<NodeSDKConfiguration> = {}) {
this._resource = configuration.resource ?? new Resource({});
this._resourceDetectors = configuration.resourceDetectors ?? [envDetector, processDetector];

this._serviceName = configuration.serviceName;

Expand Down Expand Up @@ -166,12 +169,9 @@ export class NodeSDK {
}

/** Detect resource attributes */
public async detectResources(
config?: ResourceDetectionConfig
): Promise<void> {
public async detectResources(): Promise<void> {
const internalConfig: ResourceDetectionConfig = {
detectors: [envDetector, processDetector],
...config,
detectors: this._resourceDetectors,
Comment on lines -170 to +174
Copy link
Member

Choose a reason for hiding this comment

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

this is a breaking change please add it to the changelog

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, thanks!

};

this.addResource(await detectResources(internalConfig));
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 @@ -17,7 +17,7 @@
import type { ContextManager, SpanAttributes } from '@opentelemetry/api';
import { TextMapPropagator } from '@opentelemetry/api';
import { InstrumentationOption } from '@opentelemetry/instrumentation';
import { Resource } from '@opentelemetry/resources';
import { Detector, Resource } from '@opentelemetry/resources';
import { MetricReader, View } from '@opentelemetry/sdk-metrics';
import {
Sampler,
Expand All @@ -35,6 +35,7 @@ export interface NodeSDKConfiguration {
views: View[]
instrumentations: InstrumentationOption[];
resource: Resource;
resourceDetectors: Detector[];
sampler: Sampler;
serviceName?: string;
spanProcessor: SpanProcessor;
Expand Down
36 changes: 32 additions & 4 deletions experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import * as assert from 'assert';
import * as semver from 'semver';
import * as Sinon from 'sinon';
import { NodeSDK } from '../src';
import { envDetector, processDetector } from '@opentelemetry/resources';
import {envDetector, processDetector, Resource} from '@opentelemetry/resources';


const DefaultContextManager = semver.gte(process.version, '14.8.0')
Expand Down Expand Up @@ -284,19 +284,47 @@ describe('Node SDK', () => {
delete process.env.OTEL_RESOURCE_ATTRIBUTES;
});

describe('with a buggy detector', () => {
describe('with a custom resource', () => {
it('returns a merged resource', async () => {
const sdk = new NodeSDK({
autoDetectResources: true,
resourceDetectors: [processDetector, {
async detect(): Promise<Resource> {
return new Resource({'customAttr': 'someValue'});
}
},
envDetector]
});
await sdk.detectResources();
const resource = sdk['_resource'];

assert.strictEqual(
resource.attributes['customAttr'],
'someValue'
);

assertServiceResource(resource, {
instanceId: '627cc493',
name: 'my-service',
namespace: 'default',
version: '0.0.1',
});
await sdk.detectResources({
detectors: [processDetector, {
});
});

describe('with a buggy detector', () => {
it('returns a merged resource', async () => {
const sdk = new NodeSDK({
autoDetectResources: true,
resourceDetectors: [processDetector, {
detect() {
throw new Error('Buggy detector');
}
},
envDetector]
});

await sdk.detectResources();
const resource = sdk['_resource'];

assertServiceResource(resource, {
Expand Down