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

chore!: refer to resource labels as attributes #1419

Merged
merged 5 commits into from
Aug 14, 2020
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
2 changes: 1 addition & 1 deletion packages/opentelemetry-exporter-collector/src/transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ export function toCollectorResource(
const attr = Object.assign(
{},
additionalAttributes,
resource ? resource.labels : {}
resource ? resource.attributes : {}
);
const resourceProto: opentelemetryProto.resource.v1.Resource = {
attributes: toCollectorAttributes(attr),
Expand Down
4 changes: 2 additions & 2 deletions packages/opentelemetry-exporter-jaeger/src/transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,10 @@ export function spanToThrift(span: ReadableSpan): ThriftSpan {
if (span.kind !== undefined) {
tags.push({ key: 'span.kind', value: SpanKind[span.kind] });
}
Object.keys(span.resource.labels).forEach(name =>
Object.keys(span.resource.attributes).forEach(name =>
tags.push({
key: name,
value: toTagValue(span.resource.labels[name]),
value: toTagValue(span.resource.attributes[name]),
})
);

Expand Down
4 changes: 2 additions & 2 deletions packages/opentelemetry-exporter-zipkin/src/transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ export function _toZipkinTags(
tags[statusDescriptionTagName] = status.message;
}

Object.keys(resource.labels).forEach(
name => (tags[name] = resource.labels[name])
Object.keys(resource.attributes).forEach(
name => (tags[name] = resource.attributes[name])
);

return tags;
Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry-exporter-zipkin/src/zipkin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export class ZipkinExporter implements SpanExporter {
) {
if (typeof this._serviceName !== 'string') {
this._serviceName = String(
spans[0].resource.labels[SERVICE_RESOURCE.NAME] ||
spans[0].resource.attributes[SERVICE_RESOURCE.NAME] ||
this.DEFAULT_SERVICE_NAME
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ describe('NodeTracerProvider', () => {
assert.ok(span);
assert.ok(span.resource instanceof Resource);
assert.equal(
span.resource.labels[TELEMETRY_SDK_RESOURCE.LANGUAGE],
span.resource.attributes[TELEMETRY_SDK_RESOURCE.LANGUAGE],
'nodejs'
);
});
Expand Down
22 changes: 13 additions & 9 deletions packages/opentelemetry-resources/src/Resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

import { SDK_INFO } from '@opentelemetry/core';
import { TELEMETRY_SDK_RESOURCE } from './constants';
import { ResourceLabels } from './types';
import { ResourceAttributes } from './types';

/**
* A Resource describes the entity for which a signals (metrics or trace) are
Expand Down Expand Up @@ -45,11 +45,11 @@ export class Resource {

constructor(
/**
* A dictionary of labels with string keys and values that provide information
* about the entity as numbers, strings or booleans
* TODO: Consider to add check/validation on labels.
* A dictionary of attributes with string keys and values that provide
* information about the entity as numbers, strings or booleans
* TODO: Consider to add check/validation on attributes.
*/
readonly labels: ResourceLabels
readonly attributes: ResourceAttributes
) {}

/**
Expand All @@ -61,10 +61,14 @@ export class Resource {
* @returns the newly merged Resource.
*/
merge(other: Resource | null): Resource {
if (!other || !Object.keys(other.labels).length) return this;
if (!other || !Object.keys(other.attributes).length) return this;

// Labels from resource overwrite labels from other resource.
const mergedLabels = Object.assign({}, other.labels, this.labels);
return new Resource(mergedLabels);
// Attributes from resource overwrite attributes from other resource.
const mergedAttributes = Object.assign(
{},
other.attributes,
this.attributes
);
return new Resource(mergedAttributes);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ export const detectResources = async (
const logResources = (logger: Logger, resources: Array<Resource>) => {
resources.forEach((resource, index) => {
// Print only populated resources
if (Object.keys(resource.labels).length > 0) {
const resourceDebugString = util.inspect(resource.labels, {
if (Object.keys(resource.attributes).length > 0) {
const resourceDebugString = util.inspect(resource.attributes, {
depth: 2,
breakLength: Infinity,
sorted: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class AwsEc2Detector implements Detector {
/**
* Attempts to connect and obtain an AWS instance Identity document. If the
* connection is succesful it returns a promise containing a {@link Resource}
* populated with instance metadata as labels. Returns a promise containing an
* populated with instance metadata. Returns a promise containing an
* empty {@link Resource} if the connection or parsing of the identity
* document fails.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,21 @@
*/

import { Resource } from '../../../Resource';
import { Detector, ResourceLabels } from '../../../types';
import { Detector, ResourceAttributes } from '../../../types';
import { ResourceDetectionConfigWithLogger } from '../../../config';

/**
* EnvDetector can be used to detect the presence of and create a Resource
* from the OTEL_RESOURCE_LABELS environment variable.
* from the OTEL_RESOURCE_ATTRIBUTES environment variable.
*/
class EnvDetector implements Detector {
// Type, label keys, and label values should not exceed 256 characters.
// Type, attribute keys, and attribute values should not exceed 256 characters.
private readonly _MAX_LENGTH = 255;

// OTEL_RESOURCE_LABELS is a comma-separated list of labels.
// OTEL_RESOURCE_ATTRIBUTES is a comma-separated list of attributes.
private readonly _COMMA_SEPARATOR = ',';

// OTEL_RESOURCE_LABELS contains key value pair separated by '='.
// OTEL_RESOURCE_ATTRIBUTES contains key value pair separated by '='.
private readonly _LABEL_KEY_VALUE_SPLITTER = '=';

private readonly _ERROR_MESSAGE_INVALID_CHARS =
Expand All @@ -43,50 +43,55 @@ class EnvDetector implements Detector {
' characters.';

/**
* Returns a {@link Resource} populated with labels from the
* OTEL_RESOURCE_LABELS environment variable. Note this is an async function
* to conform to the Detector interface.
* Returns a {@link Resource} populated with attributes from the
* OTEL_RESOURCE_ATTRIBUTES environment variable. Note this is an async
* function to conform to the Detector interface.
*
* @param config The resource detection config with a required logger
*/
async detect(config: ResourceDetectionConfigWithLogger): Promise<Resource> {
try {
const labelString = process.env.OTEL_RESOURCE_LABELS;
if (!labelString) {
const rawAttributes = process.env.OTEL_RESOURCE_ATTRIBUTES;
Copy link
Member

@obecny obecny Aug 13, 2020

Choose a reason for hiding this comment

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

I'm wondering one thing here, above all should this be a part of environment instead of accessing process.env directly. I know this under platform node, but still I would be in favour of getting env variables through getEnv(), I'm fine to do it in next PR as it was already like this before, WDYT ?

Copy link
Member

Choose a reason for hiding this comment

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

I agree, but I think it is outside the scope of this PR.

if (!rawAttributes) {
config.logger.debug(
'EnvDetector failed: Environment variable "OTEL_RESOURCE_LABELS" is missing.'
'EnvDetector failed: Environment variable "OTEL_RESOURCE_ATTRIBUTES" is missing.'
);
return Resource.empty();
}
const labels = this._parseResourceLabels(
process.env.OTEL_RESOURCE_LABELS
);
return new Resource(labels);
const attributes = this._parseResourceAttributes(rawAttributes);
return new Resource(attributes);
} catch (e) {
config.logger.debug(`EnvDetector failed: ${e.message}`);
return Resource.empty();
}
}

/**
* Creates a label map from the OTEL_RESOURCE_LABELS environment variable.
* Creates an attribute map from the OTEL_RESOURCE_ATTRIBUTES environment
* variable.
*
* OTEL_RESOURCE_LABELS: A comma-separated list of labels describing the
* source in more detail, e.g. “key1=val1,key2=val2”. Domain names and paths
* are accepted as label keys. Values may be quoted or unquoted in general. If
* a value contains whitespaces, =, or " characters, it must always be quoted.
* OTEL_RESOURCE_ATTRIBUTES: A comma-separated list of attributes describing
* the source in more detail, e.g. “key1=val1,key2=val2”. Domain names and
* paths are accepted as attribute keys. Values may be quoted or unquoted in
* general. If a value contains whitespaces, =, or " characters, it must
* always be quoted.
*
* @param rawEnvLabels The resource labels as a comma-seperated list
* @param rawEnvAttributes The resource attributes as a comma-seperated list
* of key/value pairs.
* @returns The sanitized resource labels.
* @returns The sanitized resource attributes.
*/
private _parseResourceLabels(rawEnvLabels?: string): ResourceLabels {
if (!rawEnvLabels) return {};
private _parseResourceAttributes(
rawEnvAttributes?: string
): ResourceAttributes {
if (!rawEnvAttributes) return {};

const labels: ResourceLabels = {};
const rawLabels: string[] = rawEnvLabels.split(this._COMMA_SEPARATOR, -1);
for (const rawLabel of rawLabels) {
const keyValuePair: string[] = rawLabel.split(
const attributes: ResourceAttributes = {};
const rawAttributes: string[] = rawEnvAttributes.split(
this._COMMA_SEPARATOR,
-1
);
for (const rawAttribute of rawAttributes) {
const keyValuePair: string[] = rawAttribute.split(
this._LABEL_KEY_VALUE_SPLITTER,
-1
);
Expand All @@ -103,9 +108,9 @@ class EnvDetector implements Detector {
if (!this._isValid(value)) {
throw new Error(`Label value ${this._ERROR_MESSAGE_INVALID_VALUE}`);
}
labels[key] = value;
attributes[key] = value;
}
return labels;
return attributes;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import * as os from 'os';
import * as gcpMetadata from 'gcp-metadata';
import { Resource } from '../../../Resource';
import { Detector, ResourceLabels } from '../../../types';
import { Detector, ResourceAttributes } from '../../../types';
import {
CLOUD_RESOURCE,
HOST_RESOURCE,
Expand All @@ -35,7 +35,7 @@ class GcpDetector implements Detector {
/**
* Attempts to connect and obtain instance configuration data from the GCP metadata service.
* If the connection is succesful it returns a promise containing a {@link Resource}
* populated with instance metadata as labels. Returns a promise containing an
* populated with instance metadata. Returns a promise containing an
* empty {@link Resource} if the connection or parsing of the metadata fails.
*
* @param config The resource detection config with a required logger
Expand All @@ -53,24 +53,27 @@ class GcpDetector implements Detector {
this._getClusterName(),
]);

const labels: ResourceLabels = {};
labels[CLOUD_RESOURCE.ACCOUNT_ID] = projectId;
labels[HOST_RESOURCE.ID] = instanceId;
labels[CLOUD_RESOURCE.ZONE] = zoneId;
labels[CLOUD_RESOURCE.PROVIDER] = 'gcp';
const attributes: ResourceAttributes = {};
attributes[CLOUD_RESOURCE.ACCOUNT_ID] = projectId;
attributes[HOST_RESOURCE.ID] = instanceId;
attributes[CLOUD_RESOURCE.ZONE] = zoneId;
attributes[CLOUD_RESOURCE.PROVIDER] = 'gcp';

if (process.env.KUBERNETES_SERVICE_HOST)
this._addK8sLabels(labels, clusterName);
this._addK8sAttributes(attributes, clusterName);

return new Resource(labels);
return new Resource(attributes);
}

/** Add resource labels for K8s */
private _addK8sLabels(labels: ResourceLabels, clusterName: string): void {
labels[K8S_RESOURCE.CLUSTER_NAME] = clusterName;
labels[K8S_RESOURCE.NAMESPACE_NAME] = process.env.NAMESPACE || '';
labels[K8S_RESOURCE.POD_NAME] = process.env.HOSTNAME || os.hostname();
labels[CONTAINER_RESOURCE.NAME] = process.env.CONTAINER_NAME || '';
/** Add resource attributes for K8s */
private _addK8sAttributes(
attributes: ResourceAttributes,
clusterName: string
): void {
attributes[K8S_RESOURCE.CLUSTER_NAME] = clusterName;
attributes[K8S_RESOURCE.NAMESPACE_NAME] = process.env.NAMESPACE || '';
attributes[K8S_RESOURCE.POD_NAME] = process.env.HOSTNAME || os.hostname();
attributes[CONTAINER_RESOURCE.NAME] = process.env.CONTAINER_NAME || '';
}

/** Gets project id from GCP project metadata. */
Expand Down
4 changes: 2 additions & 2 deletions packages/opentelemetry-resources/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
import { Resource } from './Resource';
import { ResourceDetectionConfigWithLogger } from './config';

/** Interface for Resource labels */
export interface ResourceLabels {
/** Interface for Resource attributes */
export interface ResourceAttributes {
[key: string]: number | string | boolean;
}

Expand Down
20 changes: 10 additions & 10 deletions packages/opentelemetry-resources/test/Resource.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,37 +44,37 @@ describe('Resource', () => {
'k8s.io/location': 'location',
});
const actualResource = resource1.merge(resource2);
assert.strictEqual(Object.keys(actualResource.labels).length, 5);
assert.strictEqual(Object.keys(actualResource.attributes).length, 5);
assert.deepStrictEqual(actualResource, expectedResource);
});

it('should return merged resource when collision in labels', () => {
it('should return merged resource when collision in attributes', () => {
const expectedResource = new Resource({
'k8s.io/container/name': 'c1',
'k8s.io/namespace/name': 'default',
'k8s.io/pod/name': 'pod-xyz-123',
'k8s.io/location': 'location1',
});
const actualResource = resource1.merge(resource3);
assert.strictEqual(Object.keys(actualResource.labels).length, 4);
assert.strictEqual(Object.keys(actualResource.attributes).length, 4);
assert.deepStrictEqual(actualResource, expectedResource);
});

it('should return merged resource when first resource is empty', () => {
const actualResource = emptyResource.merge(resource2);
assert.strictEqual(Object.keys(actualResource.labels).length, 2);
assert.strictEqual(Object.keys(actualResource.attributes).length, 2);
assert.deepStrictEqual(actualResource, resource2);
});

it('should return merged resource when other resource is empty', () => {
const actualResource = resource1.merge(emptyResource);
assert.strictEqual(Object.keys(actualResource.labels).length, 3);
assert.strictEqual(Object.keys(actualResource.attributes).length, 3);
assert.deepStrictEqual(actualResource, resource1);
});

it('should return merged resource when other resource is null', () => {
const actualResource = resource1.merge(null);
assert.strictEqual(Object.keys(actualResource.labels).length, 3);
assert.strictEqual(Object.keys(actualResource.attributes).length, 3);
assert.deepStrictEqual(actualResource, resource1);
});

Expand All @@ -84,15 +84,15 @@ describe('Resource', () => {
'custom.number': 42,
'custom.boolean': true,
});
assert.equal(resource.labels['custom.string'], 'strvalue');
assert.equal(resource.labels['custom.number'], 42);
assert.equal(resource.labels['custom.boolean'], true);
assert.equal(resource.attributes['custom.string'], 'strvalue');
assert.equal(resource.attributes['custom.number'], 42);
assert.equal(resource.attributes['custom.boolean'], true);
});

describe('.empty()', () => {
it('should return an empty resource', () => {
const resource = Resource.empty();
assert.equal(Object.entries(resource.labels), 0);
assert.equal(Object.entries(resource.attributes), 0);
});

it('should return the same empty resource', () => {
Expand Down
Loading