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

fix: sanitize attributes inputs #2881

Merged
merged 5 commits into from
Apr 13, 2022
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: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ All notable changes to this project will be documented in this file.

### :bug: (Bug Fix)

* fix: sanitize attributes inputs #2881 @legendecas

### :books: (Refine Doc)

* docs(sdk): update earliest support node version #2860 @svetlanabrennan
Expand Down
36 changes: 23 additions & 13 deletions packages/opentelemetry-core/src/common/attributes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,30 +13,40 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import { SpanAttributeValue, SpanAttributes } from '@opentelemetry/api';

export function sanitizeAttributes(attributes: unknown): SpanAttributes {
const out: SpanAttributes = {};
import { diag, AttributeValue, Attributes } from '@opentelemetry/api';

if (attributes == null || typeof attributes !== 'object') {
export function sanitizeAttributes(attributes: unknown): Attributes {
const out: Attributes = {};

if (typeof attributes !== 'object' || attributes == null) {
return out;
}

// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
for (const [k, v] of Object.entries(attributes!)) {
if (isAttributeValue(v)) {
if (Array.isArray(v)) {
out[k] = v.slice();
} else {
out[k] = v;
}
for (const [key, val] of Object.entries(attributes)) {
if (!isAttributeKey(key)) {
diag.warn(`Invalid attribute key: ${key}`);
continue;
}
if (!isAttributeValue(val)) {
diag.warn(`Invalid attribute value set for key: ${key}`);
continue;
}
if (Array.isArray(val)) {
out[key] = val.slice();
} else {
out[key] = val;
}
}

return out;
}

export function isAttributeValue(val: unknown): val is SpanAttributeValue {
export function isAttributeKey(key: unknown): key is string {
return typeof key === 'string' && key.length > 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor perf and minification improvements by reordering
return key && typeof === 'string';

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't find there is any big perf difference between the two flavors: https://jsbench.me/30l1ntj53a/1

TBO, I think checking the length is more explicit and easier to read.

}

export function isAttributeValue(val: unknown): val is AttributeValue {
if (val == null) {
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import {
SpanAttributes,
Attributes,
Context,
isSpanContextValid,
Link,
Expand Down Expand Up @@ -64,7 +64,7 @@ export class ParentBasedSampler implements Sampler {
traceId: string,
spanName: string,
spanKind: SpanKind,
attributes: SpanAttributes,
attributes: Attributes,
links: Link[]
): SamplingResult {
const parentContext = trace.getSpanContext(context);
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 @@ -66,9 +66,9 @@ export function toZipkinSpan(
return zipkinSpan;
}

/** Converts OpenTelemetry SpanAttributes and SpanStatus to Zipkin Tags format. */
/** Converts OpenTelemetry Attributes and SpanStatus to Zipkin Tags format. */
export function _toZipkinTags(
attributes: api.SpanAttributes,
attributes: api.Attributes,
status: api.SpanStatus,
statusCodeTagName: string,
statusErrorTagName: string,
Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry-resources/src/Resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export class Resource {
merge(other: Resource | null): Resource {
if (!other || !Object.keys(other.attributes).length) return this;

// SpanAttributes from resource overwrite attributes from other resource.
// Attributes from resource overwrite attributes from other resource.
const mergedAttributes = Object.assign(
{},
this.attributes,
Expand Down
21 changes: 12 additions & 9 deletions packages/opentelemetry-sdk-trace-base/src/Span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
InstrumentationLibrary,
isTimeInput,
timeInputToHrTime,
sanitizeAttributes,
} from '@opentelemetry/core';
import { Resource } from '@opentelemetry/resources';
import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
Expand All @@ -30,7 +31,7 @@ import { TimedEvent } from './TimedEvent';
import { Tracer } from './Tracer';
import { SpanProcessor } from './SpanProcessor';
import { SpanLimits } from './types';
import { SpanAttributeValue, Context } from '@opentelemetry/api';
import { AttributeValue, Context } from '@opentelemetry/api';
import { ExceptionEventName } from './enums';

/**
Expand All @@ -42,7 +43,7 @@ export class Span implements api.Span, ReadableSpan {
private readonly _spanContext: api.SpanContext;
readonly kind: api.SpanKind;
readonly parentSpanId?: string;
readonly attributes: api.SpanAttributes = {};
readonly attributes: api.Attributes = {};
readonly links: api.Link[] = [];
readonly events: TimedEvent[] = [];
readonly startTime: api.HrTime;
Expand Down Expand Up @@ -88,7 +89,7 @@ export class Span implements api.Span, ReadableSpan {
return this._spanContext;
}

setAttribute(key: string, value?: SpanAttributeValue): this;
setAttribute(key: string, value?: AttributeValue): this;
setAttribute(key: string, value: unknown): this {
if (value == null || this._isSpanEnded()) return this;
if (key.length === 0) {
Expand All @@ -111,7 +112,7 @@ export class Span implements api.Span, ReadableSpan {
return this;
}

setAttributes(attributes: api.SpanAttributes): this {
setAttributes(attributes: api.Attributes): this {
for (const [k, v] of Object.entries(attributes)) {
this.setAttribute(k, v);
}
Expand All @@ -127,7 +128,7 @@ export class Span implements api.Span, ReadableSpan {
*/
addEvent(
name: string,
attributesOrStartTime?: api.SpanAttributes | api.TimeInput,
attributesOrStartTime?: api.Attributes | api.TimeInput,
startTime?: api.TimeInput
): this {
if (this._isSpanEnded()) return this;
Expand All @@ -148,9 +149,11 @@ export class Span implements api.Span, ReadableSpan {
if (typeof startTime === 'undefined') {
startTime = hrTime();
}

const attributes = sanitizeAttributes(attributesOrStartTime);
this.events.push({
name,
attributes: attributesOrStartTime as api.SpanAttributes,
attributes,
time: timeInputToHrTime(startTime),
});
return this;
Expand Down Expand Up @@ -193,7 +196,7 @@ export class Span implements api.Span, ReadableSpan {
}

recordException(exception: api.Exception, time: api.TimeInput = hrTime()): void {
const attributes: api.SpanAttributes = {};
const attributes: api.Attributes = {};
if (typeof exception === 'string') {
attributes[SemanticAttributes.EXCEPTION_MESSAGE] = exception;
} else if (exception) {
Expand All @@ -217,7 +220,7 @@ export class Span implements api.Span, ReadableSpan {
attributes[SemanticAttributes.EXCEPTION_TYPE] ||
attributes[SemanticAttributes.EXCEPTION_MESSAGE]
) {
this.addEvent(ExceptionEventName, attributes as api.SpanAttributes, time);
this.addEvent(ExceptionEventName, attributes, time);
} else {
api.diag.warn(`Failed to record an exception ${exception}`);
}
Expand Down Expand Up @@ -260,7 +263,7 @@ export class Span implements api.Span, ReadableSpan {
* @param value Attribute value
* @returns truncated attribute value if required, otherwise same value
*/
private _truncateToSize(value: SpanAttributeValue): SpanAttributeValue {
private _truncateToSize(value: AttributeValue): AttributeValue {
const limit = this._attributeValueLengthLimit;
// Check limit
if (limit <= 0) {
Expand Down
4 changes: 2 additions & 2 deletions packages/opentelemetry-sdk-trace-base/src/TimedEvent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import { HrTime, SpanAttributes } from '@opentelemetry/api';
import { HrTime, Attributes } from '@opentelemetry/api';

/**
* Represents a timed event.
Expand All @@ -25,5 +25,5 @@ export interface TimedEvent {
/** The name of the event. */
name: string;
/** The attributes of the event. */
attributes?: SpanAttributes;
attributes?: Attributes;
}
13 changes: 10 additions & 3 deletions packages/opentelemetry-sdk-trace-base/src/Tracer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,12 @@ export class Tracer implements api.Tracer {
}

const spanKind = options.kind ?? api.SpanKind.INTERNAL;
const links = options.links ?? [];
const links = (options.links ?? []).map(link => {
return {
context: link.context,
attributes: sanitizeAttributes(link.attributes),
};
});
const attributes = sanitizeAttributes(options.attributes);
// make sampling decision
const samplingResult = this._sampler.shouldSample(
Expand Down Expand Up @@ -124,8 +129,10 @@ export class Tracer implements api.Tracer {
links,
options.startTime
);
// Set default attributes
span.setAttributes(Object.assign(attributes, samplingResult.attributes));
// Set initial span attributes. The attributes object may have been mutated
// by the sampler, so we sanitize the merged attributes before setting them.
const initAttributes = sanitizeAttributes(Object.assign(attributes, samplingResult.attributes));
span.setAttributes(initAttributes);
return span;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import {
SpanKind,
SpanStatus,
SpanAttributes,
Attributes,
HrTime,
Link,
SpanContext,
Expand All @@ -34,7 +34,7 @@ export interface ReadableSpan {
readonly startTime: HrTime;
readonly endTime: HrTime;
readonly status: SpanStatus;
readonly attributes: SpanAttributes;
readonly attributes: Attributes;
readonly links: Link[];
readonly events: TimedEvent[];
readonly duration: HrTime;
Expand Down
Loading