fix: sanitize attributes inputs #2881

legendecas · 2022-04-04T08:51:23Z

Which problem is this PR solving?

Fixes #2713

Short description of the changes

Sanitize all attributes inputs, including Link in Span constructor, and Event in Span.addEvent.

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Tracer.startSpan with invliad Link attributes.
Span.addEvent with invalid attributes.

Checklist:

Followed the style guidelines of this project
Unit tests have been added
~~Documentation has been updated~~

codecov · 2022-04-04T08:53:31Z

Codecov Report

Merging #2881 (bb24d38) into main (14bd6f9) will decrease coverage by 0.02%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##             main    #2881      +/-   ##
==========================================
- Coverage   93.33%   93.30%   -0.03%     
==========================================
  Files         165      165              
  Lines        5594     5605      +11     
  Branches     1176     1178       +2     
==========================================
+ Hits         5221     5230       +9     
- Misses        373      375       +2

Impacted Files	Coverage Δ
...metry-core/src/trace/sampler/ParentBasedSampler.ts	`83.87% <ø> (ø)`
...ges/opentelemetry-exporter-zipkin/src/transform.ts	`100.00% <ø> (ø)`
packages/opentelemetry-resources/src/Resource.ts	`100.00% <ø> (ø)`
...ckages/opentelemetry-core/src/common/attributes.ts	`93.33% <88.23%> (-3.97%)`	⬇️
packages/opentelemetry-sdk-trace-base/src/Span.ts	`99.18% <100.00%> (+<0.01%)`	⬆️
...ackages/opentelemetry-sdk-trace-base/src/Tracer.ts	`98.43% <100.00%> (+0.05%)`	⬆️
...ackages/opentelemetry-shim-opentracing/src/shim.ts	`94.25% <100.00%> (ø)`

dyladan · 2022-04-04T16:17:32Z

CHANGELOG.md

@@ -10,6 +10,8 @@ All notable changes to this project will be documented in this file.

 ### :bug: (Bug Fix)

+* [#2881](https://github.com/open-telemetry/opentelemetry-js/pull/2881) fix: sanitize attributes inputs ([@legendecas](https://githuc.com/legendecas))


githuc :)

Suggested change

* [#2881](https://github.com/open-telemetry/opentelemetry-js/pull/2881) fix: sanitize attributes inputs ([@legendecas](https://githuc.com/legendecas))

* [#2881](https://github.com/open-telemetry/opentelemetry-js/pull/2881) fix: sanitize attributes inputs ([@legendecas](https://github.com/legendecas))

I wonder if we should consider a less elaborate changelog format?

This might be good enough? Maybe name/handle is even optional?

* Change explanation here #000 @dyladan

I'd agree the current form is kinda verbose :) I'd be happy to shorten the entries.

@dyladan updated :)

morigs · 2022-04-04T16:47:54Z

As far as I understand, the invalid attributes will be dropped silently. Perhaps this should be documented somewhere, because this can cause a lot of confusion imo. Or perhaps it's possible to log an attribute's dropping event?

legendecas · 2022-04-04T16:52:12Z

As far as I understand, the invalid attributes will be dropped silently. Perhaps this should be documented somewhere, because this can cause a lot of confusion imo. Or perhaps it's possible to log an attribute's dropping event?

We're already dropping invalid attributes at Tracer.startSpan already: https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-sdk-trace-base/src/Tracer.ts#L96. However, when setting attributes with Span.setAttribute or Span.setAttributes, the dropped attribute is been logged: https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-sdk-trace-base/src/Span.ts#L99.

I'm fine with adding more diagnostics.

morigs · 2022-04-04T18:59:57Z

Yeah, this is exactly what I mean. IMO it's a good idea)

MSNev · 2022-04-06T16:51:02Z

packages/opentelemetry-core/src/common/attributes.ts

    }
  }

  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;


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

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.

vmarchaud

apart from nit lgtm

packages/opentelemetry-core/src/common/attributes.ts

dyladan

Sorry I thought I already approved this. LGTM

legendecas requested a review from a team April 4, 2022 08:51

legendecas force-pushed the attributes branch from 8f44059 to d419e15 Compare April 4, 2022 15:57

fix: sanitize attributes inputs

9420bcc

legendecas force-pushed the attributes branch from d419e15 to 9420bcc Compare April 4, 2022 16:01

dyladan reviewed Apr 4, 2022

View reviewed changes

fixup!

cabbee6

MSNev reviewed Apr 6, 2022

View reviewed changes

legendecas mentioned this pull request Apr 6, 2022

docs: simplify changelog entry format #2889

Merged

3 tasks

dyladan mentioned this pull request Apr 6, 2022

Replace use of api.SpanAttributes with api.Attributes #2674

Closed

dyladan linked an issue Apr 6, 2022 that may be closed by this pull request

Replace use of api.SpanAttributes with api.Attributes #2674

Closed

vmarchaud reviewed Apr 11, 2022

View reviewed changes

packages/opentelemetry-core/src/common/attributes.ts Outdated Show resolved Hide resolved

legendecas added 2 commits April 11, 2022 19:18

fixup!

fca32af

Merge branch 'main' into attributes

7d911d5

vmarchaud approved these changes Apr 11, 2022

View reviewed changes

vmarchaud requested review from MSNev and dyladan April 11, 2022 12:24

dyladan approved these changes Apr 11, 2022

View reviewed changes

rauno56 approved these changes Apr 13, 2022

View reviewed changes

Merge branch 'main' into attributes

bb24d38

vmarchaud merged commit 83355af into open-telemetry:main Apr 13, 2022

legendecas deleted the attributes branch April 13, 2022 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: sanitize attributes inputs #2881

fix: sanitize attributes inputs #2881

legendecas commented Apr 4, 2022

codecov bot commented Apr 4, 2022 •

edited

Loading

dyladan Apr 4, 2022

dyladan Apr 4, 2022

legendecas Apr 4, 2022

legendecas Apr 6, 2022

morigs commented Apr 4, 2022

legendecas commented Apr 4, 2022 •

edited

Loading

morigs commented Apr 4, 2022

MSNev Apr 6, 2022

legendecas Apr 6, 2022

vmarchaud left a comment

dyladan left a comment

		@@ -10,6 +10,8 @@ All notable changes to this project will be documented in this file.

		### :bug: (Bug Fix)

		* [#2881](https://github.com/open-telemetry/opentelemetry-js/pull/2881) fix: sanitize attributes inputs ([@legendecas](https://githuc.com/legendecas))

fix: sanitize attributes inputs #2881

fix: sanitize attributes inputs #2881

Conversation

legendecas commented Apr 4, 2022

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

codecov bot commented Apr 4, 2022 • edited Loading

Codecov Report

dyladan Apr 4, 2022

Choose a reason for hiding this comment

dyladan Apr 4, 2022

Choose a reason for hiding this comment

legendecas Apr 4, 2022

Choose a reason for hiding this comment

legendecas Apr 6, 2022

Choose a reason for hiding this comment

morigs commented Apr 4, 2022

legendecas commented Apr 4, 2022 • edited Loading

morigs commented Apr 4, 2022

MSNev Apr 6, 2022

Choose a reason for hiding this comment

legendecas Apr 6, 2022

Choose a reason for hiding this comment

vmarchaud left a comment

Choose a reason for hiding this comment

dyladan left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 4, 2022 •

edited

Loading

legendecas commented Apr 4, 2022 •

edited

Loading