diff --git a/api/src/common/AnyValue.ts b/api/src/common/AnyValue.ts new file mode 100644 index 00000000000..b408e1a5ad7 --- /dev/null +++ b/api/src/common/AnyValue.ts @@ -0,0 +1,30 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * AnyValue can be: + * - a string, number, boolean value + * - a byte array (Uint8Array) + * - array of any value + * - map from string to any value + * - empty value + * + * https://opentelemetry.io/docs/specs/otel/common/#anyvalue + * + * @since 1.3.0 + */ +export type AnyValue = + | string + | number + | boolean + | Uint8Array + | Array + | AnyValueMap + | null + | undefined; + +interface AnyValueMap { + [attributeKey: string]: AnyValue; +} diff --git a/api/src/common/Attributes.ts b/api/src/common/Attributes.ts index 75db2203d35..222a31f14ab 100644 --- a/api/src/common/Attributes.ts +++ b/api/src/common/Attributes.ts @@ -3,28 +3,19 @@ * SPDX-License-Identifier: Apache-2.0 */ +import type { AnyValue } from './AnyValue'; + /** - * Attributes is a map from string to attribute values. - * - * Note: only the own enumerable keys are counted as valid attribute keys. + * Attributes is a mapping from string to attribute values. + * https://opentelemetry.io/docs/specs/otel/common/#attribute-collections * * @since 1.3.0 */ export interface Attributes { - [attributeKey: string]: AttributeValue | undefined; + [attributeKey: string]: AnyValue; } /** - * Attribute values may be any non-nullish primitive value except an object. - * - * null or undefined attribute values are invalid and will result in undefined behavior. - * - * @since 1.3.0 + * @deprecated use AnyValue */ -export type AttributeValue = - | string - | number - | boolean - | Array - | Array - | Array; +export type AttributeValue = AnyValue; diff --git a/api/src/index.ts b/api/src/index.ts index eb9e80e0e73..03d88aaac9f 100644 --- a/api/src/index.ts +++ b/api/src/index.ts @@ -15,6 +15,7 @@ export type { export { baggageEntryMetadataFromString } from './baggage/utils'; export type { Exception } from './common/Exception'; export type { HrTime, TimeInput } from './common/Time'; +export type { AnyValue } from './common/AnyValue'; export type { Attributes, AttributeValue } from './common/Attributes'; // Context APIs @@ -71,13 +72,8 @@ export type { export type { PropagationAPI } from './api/propagation'; // Trace APIs -export type { SpanAttributes, SpanAttributeValue } from './trace/attributes'; export type { Link } from './trace/link'; export { ProxyTracer, type TracerDelegator } from './trace/ProxyTracer'; -// TODO: Remove ProxyTracerProvider export in the next major version. -export { ProxyTracerProvider } from './trace/ProxyTracerProvider'; -export type { Sampler } from './trace/Sampler'; -export { SamplingDecision, type SamplingResult } from './trace/SamplingResult'; export type { SpanContext } from './trace/span_context'; export { SpanKind } from './trace/span_kind'; export type { Span } from './trace/span'; @@ -101,6 +97,13 @@ export { } from './trace/invalid-span-constants'; export type { TraceAPI } from './api/trace'; +// Deprecated Trace APIs +export type { SpanAttributes, SpanAttributeValue } from './trace/attributes'; +// TODO: Remove ProxyTracerProvider export in the next major version. +export { ProxyTracerProvider } from './trace/ProxyTracerProvider'; +export type { Sampler } from './trace/Sampler'; +export { SamplingDecision, type SamplingResult } from './trace/SamplingResult'; + // Split module-level variable definition into separate files to allow // tree-shaking on each api instance. import { context } from './context-api'; diff --git a/api/src/metrics/Meter.ts b/api/src/metrics/Meter.ts index 34d6e60c173..cd9bc7be119 100644 --- a/api/src/metrics/Meter.ts +++ b/api/src/metrics/Meter.ts @@ -3,12 +3,12 @@ * SPDX-License-Identifier: Apache-2.0 */ +import type { Attributes } from '../common/Attributes'; import type { BatchObservableCallback, Counter, Gauge, Histogram, - MetricAttributes, MetricOptions, Observable, ObservableCounter, @@ -44,7 +44,7 @@ export interface Meter { * @param name the name of the metric. * @param [options] the metric options. */ - createGauge( + createGauge( name: string, options?: MetricOptions ): Gauge; @@ -54,7 +54,7 @@ export interface Meter { * @param name the name of the metric. * @param [options] the metric options. */ - createHistogram( + createHistogram( name: string, options?: MetricOptions ): Histogram; @@ -66,7 +66,7 @@ export interface Meter { * @param name the name of the metric. * @param [options] the metric options. */ - createCounter( + createCounter( name: string, options?: MetricOptions ): Counter; @@ -88,9 +88,7 @@ export interface Meter { * @param name the name of the metric. * @param [options] the metric options. */ - createUpDownCounter< - AttributesTypes extends MetricAttributes = MetricAttributes, - >( + createUpDownCounter( name: string, options?: MetricOptions ): UpDownCounter; @@ -103,9 +101,7 @@ export interface Meter { * @param name the name of the metric. * @param [options] the metric options. */ - createObservableGauge< - AttributesTypes extends MetricAttributes = MetricAttributes, - >( + createObservableGauge( name: string, options?: MetricOptions ): ObservableGauge; @@ -118,9 +114,7 @@ export interface Meter { * @param name the name of the metric. * @param [options] the metric options. */ - createObservableCounter< - AttributesTypes extends MetricAttributes = MetricAttributes, - >( + createObservableCounter( name: string, options?: MetricOptions ): ObservableCounter; @@ -134,7 +128,7 @@ export interface Meter { * @param [options] the metric options. */ createObservableUpDownCounter< - AttributesTypes extends MetricAttributes = MetricAttributes, + AttributesTypes extends Attributes = Attributes, >( name: string, options?: MetricOptions @@ -154,9 +148,7 @@ export interface Meter { * @param callback the batch observable callback * @param observables the observables associated with this batch observable callback */ - addBatchObservableCallback< - AttributesTypes extends MetricAttributes = MetricAttributes, - >( + addBatchObservableCallback( callback: BatchObservableCallback, observables: Observable[] ): void; @@ -171,7 +163,7 @@ export interface Meter { * @param observables the observables associated with this batch observable callback */ removeBatchObservableCallback< - AttributesTypes extends MetricAttributes = MetricAttributes, + AttributesTypes extends Attributes = Attributes, >( callback: BatchObservableCallback, observables: Observable[] diff --git a/api/src/metrics/Metric.ts b/api/src/metrics/Metric.ts index dd00e6db41a..d8ccb6a75fa 100644 --- a/api/src/metrics/Metric.ts +++ b/api/src/metrics/Metric.ts @@ -3,7 +3,8 @@ * SPDX-License-Identifier: Apache-2.0 */ -import type { Attributes, AttributeValue } from '../common/Attributes'; +import type { AnyValue } from '../common/AnyValue'; +import type { Attributes } from '../common/Attributes'; import type { Context } from '../context/types'; /** @@ -79,9 +80,7 @@ export enum ValueType { * * @since 1.3.0 */ -export interface Counter< - AttributesTypes extends MetricAttributes = MetricAttributes, -> { +export interface Counter { /** * Increment value of counter by the input. Inputs must not be negative. */ @@ -92,7 +91,7 @@ export interface Counter< * @since 1.3.0 */ export interface UpDownCounter< - AttributesTypes extends MetricAttributes = MetricAttributes, + AttributesTypes extends Attributes = Attributes, > { /** * Increment value of counter by the input. Inputs may be negative. @@ -103,9 +102,7 @@ export interface UpDownCounter< /** * @since 1.9.0 */ -export interface Gauge< - AttributesTypes extends MetricAttributes = MetricAttributes, -> { +export interface Gauge { /** * Records a measurement. */ @@ -115,9 +112,7 @@ export interface Gauge< /** * @since 1.3.0 */ -export interface Histogram< - AttributesTypes extends MetricAttributes = MetricAttributes, -> { +export interface Histogram { /** * Records a measurement. Value of the measurement must not be negative. */ @@ -134,7 +129,7 @@ export type MetricAttributes = Attributes; * @deprecated please use {@link AttributeValue} * @since 1.3.0 */ -export type MetricAttributeValue = AttributeValue; +export type MetricAttributeValue = AnyValue; /** * Interface that is being used in callback function for Observable Metric. @@ -142,7 +137,7 @@ export type MetricAttributeValue = AttributeValue; * @since 1.3.0 */ export interface ObservableResult< - AttributesTypes extends MetricAttributes = MetricAttributes, + AttributesTypes extends Attributes = Attributes, > { /** * Observe a measurement of the value associated with the given attributes. @@ -163,7 +158,7 @@ export interface ObservableResult< * Interface that is being used in batch observable callback function. */ export interface BatchObservableResult< - AttributesTypes extends MetricAttributes = MetricAttributes, + AttributesTypes extends Attributes = Attributes, > { /** * Observe a measurement of the value associated with the given attributes. @@ -188,7 +183,7 @@ export interface BatchObservableResult< * @since 1.3.0 */ export type ObservableCallback< - AttributesTypes extends MetricAttributes = MetricAttributes, + AttributesTypes extends Attributes = Attributes, > = ( observableResult: ObservableResult ) => void | Promise; @@ -199,7 +194,7 @@ export type ObservableCallback< * @since 1.3.0 */ export type BatchObservableCallback< - AttributesTypes extends MetricAttributes = MetricAttributes, + AttributesTypes extends Attributes = Attributes, > = ( observableResult: BatchObservableResult ) => void | Promise; @@ -207,9 +202,7 @@ export type BatchObservableCallback< /** * @since 1.3.0 */ -export interface Observable< - AttributesTypes extends MetricAttributes = MetricAttributes, -> { +export interface Observable { /** * Sets up a function that will be called whenever a metric collection is initiated. * @@ -226,18 +219,16 @@ export interface Observable< /** * @since 1.3.0 */ -export type ObservableCounter< - AttributesTypes extends MetricAttributes = MetricAttributes, -> = Observable; +export type ObservableCounter = + Observable; /** * @since 1.3.0 */ export type ObservableUpDownCounter< - AttributesTypes extends MetricAttributes = MetricAttributes, + AttributesTypes extends Attributes = Attributes, > = Observable; /** * @since 1.3.0 */ -export type ObservableGauge< - AttributesTypes extends MetricAttributes = MetricAttributes, -> = Observable; +export type ObservableGauge = + Observable; diff --git a/api/src/metrics/NoopMeter.ts b/api/src/metrics/NoopMeter.ts index 9fe7adc8fea..addfcc63b1a 100644 --- a/api/src/metrics/NoopMeter.ts +++ b/api/src/metrics/NoopMeter.ts @@ -3,13 +3,13 @@ * SPDX-License-Identifier: Apache-2.0 */ +import type { Attributes } from '../common/Attributes'; import type { Meter } from './Meter'; import type { BatchObservableCallback, Counter, Gauge, Histogram, - MetricAttributes, MetricOptions, Observable, ObservableCallback, @@ -101,22 +101,22 @@ export class NoopMeter implements Meter { export class NoopMetric {} export class NoopCounterMetric extends NoopMetric implements Counter { - add(_value: number, _attributes: MetricAttributes): void {} + add(_value: number, _attributes: Attributes): void {} } export class NoopUpDownCounterMetric extends NoopMetric implements UpDownCounter { - add(_value: number, _attributes: MetricAttributes): void {} + add(_value: number, _attributes: Attributes): void {} } export class NoopGaugeMetric extends NoopMetric implements Gauge { - record(_value: number, _attributes: MetricAttributes): void {} + record(_value: number, _attributes: Attributes): void {} } export class NoopHistogramMetric extends NoopMetric implements Histogram { - record(_value: number, _attributes: MetricAttributes): void {} + record(_value: number, _attributes: Attributes): void {} } export class NoopObservableMetric { diff --git a/api/src/trace/NonRecordingSpan.ts b/api/src/trace/NonRecordingSpan.ts index 0c5d7fe59dd..30a5ccaa4d7 100644 --- a/api/src/trace/NonRecordingSpan.ts +++ b/api/src/trace/NonRecordingSpan.ts @@ -3,9 +3,9 @@ * SPDX-License-Identifier: Apache-2.0 */ +import type { Attributes } from '../common/Attributes'; import type { Exception } from '../common/Exception'; import type { TimeInput } from '../common/Time'; -import type { SpanAttributes } from './attributes'; import { INVALID_SPAN_CONTEXT } from './invalid-span-constants'; import type { Span } from './span'; import type { SpanContext } from './span_context'; @@ -35,12 +35,12 @@ export class NonRecordingSpan implements Span { } // By default does nothing - setAttributes(_attributes: SpanAttributes): this { + setAttributes(_attributes: Attributes): this { return this; } // By default does nothing - addEvent(_name: string, _attributes?: SpanAttributes): this { + addEvent(_name: string, _attributes?: Attributes): this { return this; } diff --git a/api/src/trace/Sampler.ts b/api/src/trace/Sampler.ts index 34b6a8f4107..68b95bdf2cd 100644 --- a/api/src/trace/Sampler.ts +++ b/api/src/trace/Sampler.ts @@ -3,8 +3,8 @@ * SPDX-License-Identifier: Apache-2.0 */ +import type { Attributes } from '../common/Attributes'; import type { Context } from '../context/types'; -import type { SpanAttributes } from './attributes'; import type { Link } from './link'; import type { SamplingResult } from './SamplingResult'; import type { SpanKind } from './span_kind'; @@ -27,7 +27,7 @@ export interface Sampler { * span to be created starts a new trace. * @param spanName of the span to be created. * @param spanKind of the span to be created. - * @param attributes Initial set of SpanAttributes for the Span being constructed. + * @param attributes Initial set of Attributes for the Span being constructed. * @param links Collection of links that will be associated with the Span to * be created. Typically useful for batch operations. * @returns a {@link SamplingResult}. @@ -37,7 +37,7 @@ export interface Sampler { traceId: string, spanName: string, spanKind: SpanKind, - attributes: SpanAttributes, + attributes: Attributes, links: Link[] ): SamplingResult; diff --git a/api/src/trace/SamplingResult.ts b/api/src/trace/SamplingResult.ts index 6ed7d850748..00199319e31 100644 --- a/api/src/trace/SamplingResult.ts +++ b/api/src/trace/SamplingResult.ts @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import type { SpanAttributes } from './attributes'; +import type { Attributes } from '../common/Attributes'; import type { TraceState } from './trace_state'; /** @@ -48,7 +48,7 @@ export interface SamplingResult { * Caller may call {@link Sampler}.shouldSample any number of times and * can safely cache the returned value. */ - attributes?: Readonly; + attributes?: Readonly; /** * A {@link TraceState} that will be associated with the {@link Span} through * the new {@link SpanContext}. Samplers SHOULD return the TraceState from diff --git a/api/src/trace/attributes.ts b/api/src/trace/attributes.ts index 8a8e1c63806..8a0072b56a1 100644 --- a/api/src/trace/attributes.ts +++ b/api/src/trace/attributes.ts @@ -3,7 +3,8 @@ * SPDX-License-Identifier: Apache-2.0 */ -import type { Attributes, AttributeValue } from '../common/Attributes'; +import type { AnyValue } from '../common/AnyValue'; +import type { Attributes } from '../common/Attributes'; /** * @deprecated please use {@link Attributes} @@ -12,7 +13,7 @@ import type { Attributes, AttributeValue } from '../common/Attributes'; export type SpanAttributes = Attributes; /** - * @deprecated please use {@link AttributeValue} + * @deprecated please use {@link AnyValue} * @since 1.0.0 */ -export type SpanAttributeValue = AttributeValue; +export type SpanAttributeValue = AnyValue; diff --git a/api/src/trace/link.ts b/api/src/trace/link.ts index d748ff97d58..e39950f3abb 100644 --- a/api/src/trace/link.ts +++ b/api/src/trace/link.ts @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import type { SpanAttributes } from './attributes'; +import type { Attributes } from '../common/Attributes'; import type { SpanContext } from './span_context'; /** @@ -26,8 +26,11 @@ import type { SpanContext } from './span_context'; export interface Link { /** The {@link SpanContext} of a linked span. */ context: SpanContext; - /** A set of {@link SpanAttributes} on the link. */ - attributes?: SpanAttributes; + /** A set of {@link Attributes} on the link. */ + attributes?: Attributes; + // XXX A bit silly/incorrect to have this dropped count on the `Link` type + // used by Span#addLink. The user is not meant to specify this count. + // We could deprecate this, and then cope in the SDK. /** Count of attributes of the link that were dropped due to collection limits */ droppedAttributesCount?: number; } diff --git a/api/src/trace/span.ts b/api/src/trace/span.ts index 1d26cb7c68d..6847ecf32af 100644 --- a/api/src/trace/span.ts +++ b/api/src/trace/span.ts @@ -3,9 +3,10 @@ * SPDX-License-Identifier: Apache-2.0 */ +import type { AnyValue } from '../common/AnyValue'; +import type { Attributes } from '../common/Attributes'; import type { Exception } from '../common/Exception'; import type { TimeInput } from '../common/Time'; -import type { SpanAttributes, SpanAttributeValue } from './attributes'; import type { SpanContext } from './span_context'; import type { SpanStatus } from './status'; import type { Link } from './link'; @@ -39,19 +40,16 @@ export interface Span { * Sets a single Attribute with the key and value passed as arguments. * * @param key the key for this attribute. - * @param value the value for this attribute. Setting a value null or - * undefined is invalid and will result in undefined behavior. + * @param value the value for this attribute. */ - setAttribute(key: string, value: SpanAttributeValue): this; + setAttribute(key: string, value: AnyValue): this; /** * Sets attributes to the span. * * @param attributes the attributes that will be added. - * null or undefined attribute values - * are invalid and will result in undefined behavior. */ - setAttributes(attributes: SpanAttributes): this; + setAttributes(attributes: Attributes): this; /** * Adds an event to the Span. @@ -64,7 +62,7 @@ export interface Span { */ addEvent( name: string, - attributesOrStartTime?: SpanAttributes | TimeInput, + attributesOrStartTime?: Attributes | TimeInput, startTime?: TimeInput ): this; diff --git a/experimental/packages/shim-opencensus/test/ShimSpan.test.ts b/experimental/packages/shim-opencensus/test/ShimSpan.test.ts index 73a03089162..c3f23e646e9 100644 --- a/experimental/packages/shim-opencensus/test/ShimSpan.test.ts +++ b/experimental/packages/shim-opencensus/test/ShimSpan.test.ts @@ -60,7 +60,6 @@ describe('ShimSpan', () => { attributes: { foo: 'bar', }, - droppedAttributesCount: 0, name: 'the annotation', }); }); @@ -81,7 +80,6 @@ describe('ShimSpan', () => { 'message.event.size.uncompressed': 12, 'message.event.type': 'SENT', }, - droppedAttributesCount: 0, name: '98', }); }); diff --git a/packages/opentelemetry-core/src/common/attributes.ts b/packages/opentelemetry-core/src/common/attributes.ts index 03fbff3e410..1592033d962 100644 --- a/packages/opentelemetry-core/src/common/attributes.ts +++ b/packages/opentelemetry-core/src/common/attributes.ts @@ -6,86 +6,291 @@ import type { AttributeValue, Attributes } from '@opentelemetry/api'; import { diag } from '@opentelemetry/api'; -export function sanitizeAttributes(attributes: unknown): Attributes { - const out: Attributes = {}; +/** + * Return a valid {@link Attributes} object from the given object. + * + * Limitations: + * - This does not handle attribute limits. + * https://opentelemetry.io/docs/specs/otel/common/#attribute-limits + * - This does not return a number of dropped attributes, needed for + * handling `droppedAttributesCount` OTLP fields. + * + * @deprecated use normalizeAttributes + */ +export function sanitizeAttributes(obj: unknown): Attributes { + const { attributes } = normalizeAttributes(obj); + return attributes ?? {}; +} + +/** + * Validates if a value is a valid AttributeValue ("AnyValue" in the spec): + * https://opentelemetry.io/docs/specs/otel/common/#anyvalue + * + * This also returns false if a cicular reference is detected. + * + * @param val - The value to validate + * @returns true if the value is a valid AttributeValue, false otherwise + */ +export function isAttributeValue(val: unknown): val is AttributeValue { + return isAttributeValueInternal(val, new WeakSet()); +} +// XXX rename this to `isAnyValue`, and deprecated isAttributeValue - if (typeof attributes !== 'object' || attributes == null) { - return out; +function isAttributeValueInternal( + val: unknown, + visited: WeakSet +): val is AttributeValue { + // null and undefined are explicitly allowed + if (val == null) { + return true; } - for (const key in attributes) { - if (!Object.prototype.hasOwnProperty.call(attributes, key)) { - continue; - } - if (!isAttributeKey(key)) { - diag.warn(`Invalid attribute key: ${key}`); - continue; - } - const val = (attributes as Record)[key]; - if (!isAttributeValue(val)) { - diag.warn(`Invalid attribute value set for key: ${key}`); - continue; + // Scalar values + if ( + typeof val === 'string' || + typeof val === 'number' || + typeof val === 'boolean' + ) { + return true; + } + + // Byte arrays + if (val instanceof Uint8Array) { + return true; + } + + // For objects and arrays, check for circular references + if (typeof val === 'object') { + if (visited.has(val as object)) { + // Circular reference detected - reject it + return false; } + visited.add(val as object); + + // Arrays (can contain any AnyValue, including heterogeneous) if (Array.isArray(val)) { - out[key] = val.slice(); - } else { - out[key] = val; + return val.every(item => isAttributeValueInternal(item, visited)); + } + + // Only accept plain objects (not built-in objects like Date, RegExp, Error, etc.) + // Check if it's a plain object by verifying its constructor is Object or it has no constructor + const obj = val as Record; + if (obj.constructor !== Object && obj.constructor !== undefined) { + return false; } + + // Objects/Maps (including empty objects) + // All object properties must be valid AnyValues + return Object.values(obj).every(item => + isAttributeValueInternal(item, visited) + ); } - return out; + return false; } -export function isAttributeKey(key: unknown): key is string { - return typeof key === 'string' && key !== ''; +// XXX not lovin' using enums. Would prefer to stick to eraseable syntax, but no requirement for that. +// Cannot use *const enum* at least, https://github.com/open-telemetry/opentelemetry-js/issues/5691 +export enum AddAttributeDecision { + DROP_INVALID = 0, + DROP_LIMIT_REACHED = 1, + ADD_NEW = 2, + ADD_OVERWRITE_EXISTING = 3, } -export function isAttributeValue(val: unknown): val is AttributeValue { - if (val == null) { - return true; +// XXX Rename to assignAttribute (a la Object.assign) perhaps? Something else? +// XXX Want a version without limits: resources and metrics are exempt: https://opentelemetry.io/docs/specs/otel/common/#exempt-entities +// XXX Having to pass around attribute count (currentAttributesCount, etc.) is a pain. Not suggesting it now, because more work, but using a Map would be nice for the API. +// XXX Consider moving diag.warn()s out to caller. +export function addAttribute( + attributes: Attributes, + currentAttributesCount: number, + countLimit: number, + valueLengthLimit: number, + key: string, + // XXX Marc had `value?`. Is that really necessary? https://github.com/open-telemetry/opentelemetry-js/pull/6573 + value: unknown +): AddAttributeDecision { + if (key.length === 0) { + diag.warn(`Invalid attribute key: ${key}`); + return AddAttributeDecision.DROP_INVALID; } - - if (Array.isArray(val)) { - return isHomogeneousAttributeValueArray(val); + if (!isAttributeValue(value)) { + diag.warn(`Invalid attribute value set for key: ${key}`); + return AddAttributeDecision.DROP_INVALID; + } + const isNewKey = !Object.hasOwn(attributes, key); + if (isNewKey && currentAttributesCount >= countLimit) { + return AddAttributeDecision.DROP_LIMIT_REACHED; } - return isValidPrimitiveAttributeValueType(typeof val); + // `truncateToSize` makes a copy of arrays and objects, which ensures + // mutation of the passed in object does not impact the serialized attributes. + attributes[key] = truncateToSize(value, valueLengthLimit); + if (isNewKey) { + return AddAttributeDecision.ADD_NEW; + } + return AddAttributeDecision.ADD_OVERWRITE_EXISTING; } -function isHomogeneousAttributeValueArray(arr: unknown[]): boolean { - let type: string | undefined; +function truncateToSize(value: AttributeValue, limit: number): AttributeValue { + // Check limit + // XXX move this limit check to the top-level. + // XXX this has the subtle side-effect that a copy of arrays/objects is NOT taken + if (limit <= 0) { + // Negative values are invalid, so do not truncate + diag.warn(`Attribute value limit must be positive, got ${limit}`); + return value; + } - for (const element of arr) { - // null/undefined elements are allowed - if (element == null) continue; - const elementType = typeof element; + // null/undefined - no truncation needed + if (value == null) { + return value; + } - if (elementType === type) { - continue; + // String + if (typeof value === 'string') { + if (value.length <= limit) { + return value; } + return value.substring(0, limit); + } - if (!type) { - if (isValidPrimitiveAttributeValueType(elementType)) { - type = elementType; - continue; - } - // encountered an invalid primitive - return false; + // Byte arrays - no truncation needed + // XXX wrong, spec says to truncate + if (value instanceof Uint8Array) { + return value; + } + + // Arrays (can contain any AnyValue types) + if (Array.isArray(value)) { + return value.map(val => truncateToSize(val, limit)); + } + + // Objects/Maps - recursively truncate nested values + if (typeof value === 'object') { + const truncatedObj: Record = {}; + for (const [k, v] of Object.entries( + value as Record + )) { + truncatedObj[k] = truncateToSize(v, limit); } + return truncatedObj; + } + + // Other types (number, boolean), no need to apply value length limit + return value; +} - return false; +/** + * Normalize attributes for use on the instrumentation scope. Drops invalid attributes and keeps track of + * how many were dropped. + * + * @param limits + * @param attributes + */ +export function normalizeAttributes( + attributes: unknown | undefined, + countLimit = Infinity, + valueLengthLimit = Infinity +): { + attributes?: Attributes; + droppedAttributesCount?: number; +} { + if (attributes == null) { + return {}; } - return true; + const normalizedAttributes: Attributes = {}; + let currentAttributesCount = 0; + let droppedAttributesCount = 0; + + for (const [key, value] of Object.entries(attributes)) { + const decision = addAttribute( + normalizedAttributes, + currentAttributesCount, + countLimit, + valueLengthLimit, + key, + value + ); + + if (decision === AddAttributeDecision.ADD_NEW) { + currentAttributesCount += 1; + } else if (decision === AddAttributeDecision.DROP_INVALID) { + droppedAttributesCount += 1; + } else if (decision === AddAttributeDecision.DROP_LIMIT_REACHED) { + droppedAttributesCount += 1; + } else { + // do nothing + } + } + + const result: ReturnType = {}; + if (currentAttributesCount > 0) { + result.attributes = normalizedAttributes; + } + if (droppedAttributesCount) { + result.droppedAttributesCount = droppedAttributesCount; + } + return result; } -function isValidPrimitiveAttributeValueType(valType: string): boolean { - switch (valType) { - case 'number': - case 'boolean': - case 'string': - return true; +// XXX adapted from Marc's https://github.com/open-telemetry/opentelemetry-js/pull/6573/changes#diff-51e43be6170821a96fa159a51a1c557810feb4ba2cf6f5c9a13ca365bda3a4c4R14-R27 +/** + * Return a normalized JSON-serializable object for an AnyValue. + * + * This is useful for creating a mapping key for SDK data structures including + * `Attributes`. For example: + * - the Metrics SDK needs to group instrument values based on Attributes + * - LoggerProvider et al need to key on InstrumentationScope data, which can + * include Attributes. + * + * Dev Notes: + * This normalization uses a [typeTag, payload] tuple. + * Using a type tag as the first element guarantees that two values can only + * produce the same tuple when they have the same type AND the same data, + * avoiding cross-type collisions such as: + * - null vs NaN vs Infinity (all become JSON `null` via JSON.stringify) + * - -0 vs 0 (both become JSON `0` via JSON.stringify) + * - string "null" vs the value null + * + * Object keys are sorted so that attribute maps with the same entries but + * different insertion orders produce the same key. + */ +export function keyObjFromAnyValue(value: AttributeValue): unknown { + if (value === undefined) { + return ['u', null]; + } + if (value === null) { + return ['n', null]; } - return false; + const valueType = typeof value; + if (valueType === 'string') { + return ['s', value]; + } + if (valueType === 'boolean') { + return ['b', value]; + } + if (valueType === 'number') { + if (Number.isNaN(value)) return ['nan', null]; + if (value === Infinity) return ['inf', null]; + if (value === -Infinity) return ['-inf', null]; + if (Object.is(value, -0)) return ['n0', null]; + return ['d', value]; + } + if (value instanceof Uint8Array) { + return ['bytes', Array.from(value)]; + } + if (Array.isArray(value)) { + return ['arr', value.map(keyObjFromAnyValue)]; + } + // AnyValueMap — sort keys for insertion-order independence + return [ + 'map', + Object.entries(value) + .sort(([a], [b]) => a.localeCompare(b)) + .map(([k, v]) => [k, keyObjFromAnyValue(v)]), + ]; } diff --git a/packages/opentelemetry-core/src/index.ts b/packages/opentelemetry-core/src/index.ts index 81456120b4a..9d6e9a9776d 100644 --- a/packages/opentelemetry-core/src/index.ts +++ b/packages/opentelemetry-core/src/index.ts @@ -6,7 +6,14 @@ export { W3CBaggagePropagator } from './baggage/propagation/W3CBaggagePropagator'; export { AnchoredClock } from './common/anchored-clock'; export type { Clock } from './common/anchored-clock'; -export { isAttributeValue, sanitizeAttributes } from './common/attributes'; +export { + isAttributeValue, + sanitizeAttributes, + normalizeAttributes, + addAttribute, // XXX perhaps change name + AddAttributeDecision, // XXX ew, enums :) + keyObjFromAnyValue, +} from './common/attributes'; export { globalErrorHandler, setGlobalErrorHandler, diff --git a/packages/opentelemetry-core/test/common/attributes.test.ts b/packages/opentelemetry-core/test/common/attributes.test.ts index 6b8870585d9..c41b5fb0e4a 100644 --- a/packages/opentelemetry-core/test/common/attributes.test.ts +++ b/packages/opentelemetry-core/test/common/attributes.test.ts @@ -24,8 +24,8 @@ describe('attributes', () => { assert.ok(isAttributeValue()); }); - it('should not allow objects', () => { - assert.ok(!isAttributeValue({})); + it('should allow objects', () => { + assert.ok(isAttributeValue({})); }); it('should allow homogeneous arrays', () => { @@ -42,18 +42,21 @@ describe('attributes', () => { assert.ok(isAttributeValue(['str1', undefined, 'str3'])); }); - it('should not allow heterogeneous arrays', () => { - assert.ok(!isAttributeValue([0, false, 2])); - assert.ok(!isAttributeValue([true, 'false', true])); - assert.ok(!isAttributeValue(['str1', 2, 'str3'])); + it('should allow heterogeneous arrays', () => { + assert.ok(isAttributeValue([0, false, 2])); + assert.ok(isAttributeValue([true, 'false', true])); + assert.ok(isAttributeValue(['str1', 2, 'str3'])); }); - it('should not allow arrays of objects or nested arrays', () => { - assert.ok(!isAttributeValue([{}])); - assert.ok(!isAttributeValue([[]])); + it('should allow arrays of objects or nested arrays', () => { + assert.ok(isAttributeValue([{}])); + assert.ok(isAttributeValue([[]])); }); + + // XXX add more tests on new AnyValue edge cases }); - describe('#sanitize', () => { + + describe('#sanitizeAttributes', () => { it('should remove invalid fields', () => { const attributes = sanitizeAttributes({ str: 'string', @@ -64,15 +67,19 @@ describe('attributes', () => { arrNum: [0, null, 1], arrBool: [false, undefined, true], mixedArr: [0, false], + uint16array: new Uint16Array([1, 2, 3]), + date: new Date(), }); assert.deepStrictEqual(attributes, { str: 'string', num: 0, bool: false, + object: {}, arrStr: ['str1', null, 'str2'], arrNum: [0, null, 1], arrBool: [false, undefined, true], + mixedArr: [0, false], }); }); @@ -92,4 +99,6 @@ describe('attributes', () => { assert.strictEqual(attributes.arr[0], 'unmodified'); }); }); + + // XXX test new core exports: addAttribute, keyObjFromAnyValue, normalizeAttributes }); diff --git a/packages/opentelemetry-sdk-trace-base/src/Span.ts b/packages/opentelemetry-sdk-trace-base/src/Span.ts index 7dab28618c2..7b63c1daaf7 100644 --- a/packages/opentelemetry-sdk-trace-base/src/Span.ts +++ b/packages/opentelemetry-sdk-trace-base/src/Span.ts @@ -24,11 +24,12 @@ import { millisToHrTime, hrTime, hrTimeDuration, - isAttributeValue, isTimeInput, isTimeInputHrTime, otperformance, - sanitizeAttributes, + normalizeAttributes, + addAttribute, + AddAttributeDecision, } from '@opentelemetry/core'; import type { Resource } from '@opentelemetry/resources'; import { @@ -95,7 +96,7 @@ export class SpanImpl implements Span { private _duration: HrTime = [-1, -1]; private readonly _spanProcessor: SpanProcessor; private readonly _spanLimits: SpanLimits; - private readonly _attributeValueLengthLimit: number; + // private readonly _attributeValueLengthLimit: number; // XXX need this? private readonly _recordEndMetrics?: () => void; private readonly _performanceStartTime: number; @@ -114,13 +115,16 @@ export class SpanImpl implements Span { now - (this._performanceStartTime + otperformance.timeOrigin); this._startTimeProvided = opts.startTime != null; this._spanLimits = opts.spanLimits; - this._attributeValueLengthLimit = - this._spanLimits.attributeValueLengthLimit ?? 0; + // XXX Why this denormalized var? Just use from spanLimits? `0` isn't a reasonable default. + // this._attributeValueLengthLimit = + // this._spanLimits.attributeValueLengthLimit ?? 0; this._spanProcessor = opts.spanProcessor; this.name = opts.name; this.parentSpanContext = opts.parentSpanContext; this.kind = opts.kind; + // XXX should be able to assign these directly. Tracer.startSpan already + // handles normalization. It should passing droppedAttributesCount too. if (opts.links) { for (const link of opts.links) { this.addLink(link); @@ -132,7 +136,17 @@ export class SpanImpl implements Span { this._recordEndMetrics = opts.recordEndMetrics; if (opts.attributes != null) { - this.setAttributes(opts.attributes); + const { attributes, droppedAttributesCount } = normalizeAttributes( + opts.attributes, + this._spanLimits.attributeCountLimit, + this._spanLimits.attributeValueLengthLimit + ); + if (attributes) { + this.attributes = attributes; + } + if (droppedAttributesCount) { + this._droppedAttributesCount += droppedAttributesCount; + } } this._spanProcessor.onStart(this, opts.context); @@ -142,37 +156,35 @@ export class SpanImpl implements Span { return this._spanContext; } + // XXX For next major: + // - Could we drop the `value?`? setAttribute() in API doesn't have it. + // - Could we drop the `unknown`? While the SDK setAttribute defensively + // handles whatever type is thrown at it, the *API* intent is that the + // called only sends `AttributeValue`. setAttribute(key: string, value?: AttributeValue): this; setAttribute(key: string, value: unknown): this { - if (value == null || this._isSpanEnded()) return this; - if (key.length === 0) { - diag.warn(`Invalid attribute key: ${key}`); - return this; - } - if (!isAttributeValue(value)) { - diag.warn(`Invalid attribute value set for key: ${key}`); - return this; - } + if (this._isSpanEnded()) return this; - const { attributeCountLimit } = this._spanLimits; - const isNewKey = !Object.prototype.hasOwnProperty.call( + const decision = addAttribute( this.attributes, - key + this._attributesCount, + this._spanLimits.attributeCountLimit ?? 128, // XXX how to ensure this is set in ctor? Hack for now + this._spanLimits.attributeValueLengthLimit ?? Infinity, // XXX hack default for now. Type should be Required + key, + value ); - if ( - attributeCountLimit !== undefined && - this._attributesCount >= attributeCountLimit && - isNewKey - ) { + // XXX Handle DROP_INVALID? + if (decision === AddAttributeDecision.DROP_LIMIT_REACHED) { this._droppedAttributesCount++; - return this; - } - - this.attributes[key] = this._truncateToSize(value); - if (isNewKey) { + if (this._droppedAttributesCount === 1) { + // Only warn once per LogRecord to avoid log spam + diag.warn('Dropping extra attributes.'); + } + } else if (decision === AddAttributeDecision.ADD_NEW) { this._attributesCount++; } + return this; } @@ -225,34 +237,17 @@ export class SpanImpl implements Span { attributesOrStartTime = undefined; } - const sanitized = sanitizeAttributes(attributesOrStartTime); - const { attributePerEventCountLimit } = this._spanLimits; - const attributes: Attributes = {}; - let droppedAttributesCount = 0; - let eventAttributesCount = 0; - - for (const attr in sanitized) { - if (!Object.prototype.hasOwnProperty.call(sanitized, attr)) { - continue; - } - const attrVal = sanitized[attr]; - if ( - attributePerEventCountLimit !== undefined && - eventAttributesCount >= attributePerEventCountLimit - ) { - droppedAttributesCount++; - continue; - } - attributes[attr] = this._truncateToSize(attrVal!); - eventAttributesCount++; - } - this.events.push({ name, - attributes, time: this._getTime(timeStamp), - droppedAttributesCount, + ...normalizeAttributes( + attributesOrStartTime, + // XXX old code is handling `attributePerEventCountLimit !== undefined`. Need we here? + this._spanLimits.attributePerEventCountLimit, + this._spanLimits.attributeValueLengthLimit + ), }); + return this; } @@ -274,37 +269,16 @@ export class SpanImpl implements Span { this._droppedLinksCount++; } - const { attributePerLinkCountLimit } = this._spanLimits; - const sanitized = sanitizeAttributes(link.attributes); - const attributes: Attributes = {}; - let droppedAttributesCount = 0; - let linkAttributesCount = 0; - - for (const attr in sanitized) { - if (!Object.prototype.hasOwnProperty.call(sanitized, attr)) { - continue; - } - const attrVal = sanitized[attr]; - if ( - attributePerLinkCountLimit !== undefined && - linkAttributesCount >= attributePerLinkCountLimit - ) { - droppedAttributesCount++; - continue; - } - attributes[attr] = this._truncateToSize(attrVal!); - linkAttributesCount++; - } - - const processedLink: Link = { context: link.context }; - if (linkAttributesCount > 0) { - processedLink.attributes = attributes; - } - if (droppedAttributesCount > 0) { - processedLink.droppedAttributesCount = droppedAttributesCount; - } + this.links.push({ + context: link.context, + ...normalizeAttributes( + link.attributes, + // XXX old code is handling `attributePerLinkCountLimit !== undefined`. Need we here? + this._spanLimits.attributePerLinkCountLimit, + this._spanLimits.attributeValueLengthLimit + ), + }); - this.links.push(processedLink); return this; } @@ -477,51 +451,4 @@ export class SpanImpl implements Span { } return this._ended; } - - // Utility function to truncate given value within size - // for value type of string, will truncate to given limit - // for type of non-string, will return same value - private _truncateToLimitUtil(value: string, limit: number): string { - if (value.length <= limit) { - return value; - } - return value.substring(0, limit); - } - - /** - * If the given attribute value is of type string and has more characters than given {@code attributeValueLengthLimit} then - * return string with truncated to {@code attributeValueLengthLimit} characters - * - * If the given attribute value is array of strings then - * return new array of strings with each element truncated to {@code attributeValueLengthLimit} characters - * - * Otherwise return same Attribute {@code value} - * - * @param value Attribute value - * @returns truncated attribute value if required, otherwise same value - */ - private _truncateToSize(value: AttributeValue): AttributeValue { - const limit = this._attributeValueLengthLimit; - // Check limit - if (limit <= 0) { - // Negative values are invalid, so do not truncate - diag.warn(`Attribute value limit must be positive, got ${limit}`); - return value; - } - - // String - if (typeof value === 'string') { - return this._truncateToLimitUtil(value, limit); - } - - // Array of strings - if (Array.isArray(value)) { - return (value as []).map(val => - typeof val === 'string' ? this._truncateToLimitUtil(val, limit) : val - ); - } - - // Other types, no need to apply value length limit - return value; - } } diff --git a/packages/opentelemetry-sdk-trace-base/src/Tracer.ts b/packages/opentelemetry-sdk-trace-base/src/Tracer.ts index 1c6a4c2a98f..f5a6963ea89 100644 --- a/packages/opentelemetry-sdk-trace-base/src/Tracer.ts +++ b/packages/opentelemetry-sdk-trace-base/src/Tracer.ts @@ -97,12 +97,17 @@ export class Tracer implements api.Tracer { } const spanKind = options.kind ?? api.SpanKind.INTERNAL; + // XXX use normalizeAttributes, keep droppedAttributesCount values, + // separate from the `links` sent to shouldSample. Then pass links + // *with* droppedAttributesCount to SpanImpl, which accepts those. + // I.e. SpanImpl type shoudl be `Array` const links = (options.links ?? []).map(link => { return { context: link.context, attributes: sanitizeAttributes(link.attributes), }; }); + // XXX use normalizeAttributes, *remove* normalization in SpanImpl ctor const attributes = sanitizeAttributes(options.attributes); // make sampling decision const samplingResult = this._sampler.shouldSample( diff --git a/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts index c3d68f4ab0e..07b513398aa 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/Span.test.ts @@ -1597,14 +1597,14 @@ describe('Span', () => { assert.strictEqual(span.events.length, 1); const [event] = span.events; assert.deepStrictEqual(event.name, 'sent'); - assert.deepStrictEqual(event.attributes, {}); + assert.deepStrictEqual(event.attributes, undefined); assert.ok(event.time[0] > 0); span.addEvent('rev', { attr1: 'value', attr2: 123, attr3: true }); assert.strictEqual(span.events.length, 2); const [event1, event2] = span.events; assert.deepStrictEqual(event1.name, 'sent'); - assert.deepStrictEqual(event1.attributes, {}); + assert.deepStrictEqual(event1.attributes, undefined); assert.ok(event1.time[0] > 0); assert.deepStrictEqual(event2.name, 'rev'); assert.deepStrictEqual(event2.attributes, { diff --git a/packages/opentelemetry-sdk-trace-base/test/common/util.ts b/packages/opentelemetry-sdk-trace-base/test/common/util.ts index af36681b8be..a26455def80 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/util.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/util.ts @@ -12,15 +12,18 @@ export const validAttributes = { 'array': ['str1', 'str2'], 'array': [1, 2], 'array': [true, false], + bytes: new Uint8Array([68, 69]), + obj: { foo: 'bar' }, + nested: { anObj: {}, anArray: [1, 2, 3] }, }; export const invalidAttributes = { - // invalid attribute type object - object: { foo: 'bar' }, - // invalid attribute inhomogenous array - 'non-homogeneous-array': [0, ''], // This empty length attribute should not be set '': 'empty-key', + // Non-Object objects, for example: + date: new Date(), + regexp: /^f./, + err: new Error('boom'), }; export function assertAssignable(val: T): asserts val is T {} diff --git a/packages/sdk-metrics/src/utils.ts b/packages/sdk-metrics/src/utils.ts index 07fb9a3b1eb..08e8a365f9b 100644 --- a/packages/sdk-metrics/src/utils.ts +++ b/packages/sdk-metrics/src/utils.ts @@ -5,6 +5,7 @@ import type { Attributes } from '@opentelemetry/api'; import type { InstrumentationScope } from '@opentelemetry/core'; +import { keyObjFromAnyValue } from '@opentelemetry/core'; export type Maybe = T | undefined; @@ -13,12 +14,13 @@ export type Maybe = T | undefined; * @param attributes user provided unordered Attributes. */ export function hashAttributes(attributes: Attributes): string { - let keys = Object.keys(attributes); + const keys = Object.keys(attributes); if (keys.length === 0) return ''; - // Return a string that is stable on key orders. - keys = keys.sort(); - return JSON.stringify(keys.map(key => [key, attributes[key]])); + // XXX Should this be defensively dropping attributes with invalid type? + + const keyObj = keyObjFromAnyValue(attributes); + return JSON.stringify(keyObj); } /** diff --git a/packages/sdk-metrics/test/utils.test.ts b/packages/sdk-metrics/test/utils.test.ts index d4ca9f75bcc..4ff3a25b807 100644 --- a/packages/sdk-metrics/test/utils.test.ts +++ b/packages/sdk-metrics/test/utils.test.ts @@ -35,6 +35,9 @@ describe('utils', () => { describe('hashAttributes', () => { it('should hash all types of attribute values', () => { + // XXX re-write this test. The goal isn't that `hashAttributes` is stable + // over time, but that two equivalent-data Attributes objects hash to the same value, + // and diff ones do not. const cases: [Attributes, string][] = [ [{ string: 'bar' }, '[["string","bar"]]'], [{ number: 1 }, '[["number",1]]'],