Merge branch 'main' into Browser_Detector_Module

open-telemetry · Nov 1, 2022 · fae5ec4 · fae5ec4
2 parents 1ed3b2b + a3e40da
commit fae5ec4
Show file tree

Hide file tree

Showing 6 changed files with 69 additions and 24 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -17,6 +17,10 @@ All notable changes to this project will be documented in this file.
   [#3327](https://github.com/open-telemetry/opentelemetry-js/pull/3327) @dyladan
 * fix(resources): fix EnvDetector throwing errors when attribute values contain spaces
   [#3295](https://github.com/open-telemetry/opentelemetry-js/issues/3295)
+* fix(trace): fix an issue which caused negative span durations in web based spans
+  [#3359](https://github.com/open-telemetry/opentelemetry-js/pull/3359) @dyladan
+* fix(resources): strict OTEL_RESOURCE_ATTRIBUTES baggage octet decoding
+  [#3341](https://github.com/open-telemetry/opentelemetry-js/pull/3341) @legendecas
 
 ### :books: (Refine Doc)
 

diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts
@@ -416,7 +416,7 @@ describe('Node SDK', () => {
 
       describe('with a faulty environment variable', () => {
         beforeEach(() => {
-          process.env.OTEL_RESOURCE_ATTRIBUTES = 'bad=~attribute';
+          process.env.OTEL_RESOURCE_ATTRIBUTES = 'bad=\\attribute';
         });
 
         it('prints correct error messages when EnvDetector has an invalid variable', async () => {

diff --git a/packages/opentelemetry-resources/src/detectors/EnvDetector.ts b/packages/opentelemetry-resources/src/detectors/EnvDetector.ts
@@ -117,7 +117,7 @@ class EnvDetector implements Detector {
       if (!this._isValid(value)) {
         throw new Error(`Attribute value ${this._ERROR_MESSAGE_INVALID_VALUE}`);
       }
-      attributes[key] = value;
+      attributes[key] = decodeURIComponent(value);
     }
     return attributes;
   }
@@ -130,13 +130,14 @@ class EnvDetector implements Detector {
    * @returns Whether the String is valid.
    */
   private _isValid(name: string): boolean {
-    return name.length <= this._MAX_LENGTH && this._isPrintableString(name);
+    return name.length <= this._MAX_LENGTH && this._isBaggageOctetString(name);
   }
 
-  private _isPrintableString(str: string): boolean {
+  // https://www.w3.org/TR/baggage/#definition
+  private _isBaggageOctetString(str: string): boolean {
     for (let i = 0; i < str.length; i++) {
-      const ch: string = str.charAt(i);
-      if (ch < ' ' || ch >= '~') {
+      const ch = str.charCodeAt(i);
+      if (ch < 0x21 || ch === 0x2C || ch === 0x3B || ch === 0x5C || ch > 0x7E) {
         return false;
       }
     }

diff --git a/packages/opentelemetry-resources/test/detectors/browser/EnvDetector.test.ts b/packages/opentelemetry-resources/test/detectors/browser/EnvDetector.test.ts
@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+import * as assert from 'assert';
 import { RAW_ENVIRONMENT } from '@opentelemetry/core';
 import { SemanticResourceAttributes } from '@opentelemetry/semantic-conventions';
 import { envDetector, Resource } from '../../../src';
@@ -27,7 +28,7 @@ describeBrowser('envDetector() on web browser', () => {
   describe('with valid env', () => {
     before(() => {
       (globalThis as typeof globalThis & RAW_ENVIRONMENT).OTEL_RESOURCE_ATTRIBUTES =
-        'webengine.name="chromium",webengine.version="99",webengine.description="Chromium"';
+        'webengine.name="chromium",webengine.version="99",webengine.description="Chromium",custom.key="custom%20value"';
     });
 
     after(() => {
@@ -41,6 +42,38 @@ describeBrowser('envDetector() on web browser', () => {
         [SemanticResourceAttributes.WEBENGINE_VERSION]: '99',
         [SemanticResourceAttributes.WEBENGINE_DESCRIPTION]: 'Chromium',
       });
+      assert.strictEqual(resource.attributes['custom.key'], 'custom value');
+    });
+  });
+
+
+  describe('with invalid env', () => {
+    const values = [
+      'webengine.description="with spaces"',
+    ];
+
+    for (const value of values) {
+      describe(`value: '${value}'`, () => {
+        before(() => {
+          (globalThis as typeof globalThis & RAW_ENVIRONMENT).OTEL_RESOURCE_ATTRIBUTES = value;
+        });
+
+        after(() => {
+          delete (globalThis as typeof globalThis & RAW_ENVIRONMENT).OTEL_RESOURCE_ATTRIBUTES;
+        });
+
+        it('should return empty resource', async () => {
+          const resource: Resource = await envDetector.detect();
+          assertEmptyResource(resource);
+        });
+      });
+    }
+  });
+
+  describe('with empty env', () => {
+    it('should return empty resource', async () => {
+      const resource: Resource = await envDetector.detect();
+      assertEmptyResource(resource);
     });
   });
 

diff --git a/packages/opentelemetry-resources/test/detectors/node/EnvDetector.test.ts b/packages/opentelemetry-resources/test/detectors/node/EnvDetector.test.ts
@@ -25,7 +25,7 @@ describeNode('envDetector() on Node.js', () => {
   describe('with valid env', () => {
     before(() => {
       process.env.OTEL_RESOURCE_ATTRIBUTES =
-        'k8s.pod.name="pod-xyz-123",k8s.cluster.name="c1",k8s.namespace.name="default",k8s.deployment.name="deployment name"';
+        'k8s.pod.name="pod-xyz-123",k8s.cluster.name="c1",k8s.namespace.name="default",k8s.deployment.name="deployment%20name"';
     });
 
     after(() => {
@@ -43,6 +43,29 @@ describeNode('envDetector() on Node.js', () => {
     });
   });
 
+  describe('with invalid env', () => {
+    const values = [
+      'k8s.deployment.name="with spaces"',
+    ];
+
+    for (const value of values) {
+      describe(`value: '${value}'`, () => {
+        before(() => {
+          process.env.OTEL_RESOURCE_ATTRIBUTES = value;
+        });
+
+        after(() => {
+          delete process.env.OTEL_RESOURCE_ATTRIBUTES;
+        });
+
+        it('should return empty resource', async () => {
+          const resource: Resource = await envDetector.detect();
+          assertEmptyResource(resource);
+        });
+      });
+    }
+  });
+
   describe('with empty env', () => {
     it('should return empty resource', async () => {
       const resource: Resource = await envDetector.detect();

diff --git a/packages/opentelemetry-sdk-trace-base/src/Tracer.ts b/packages/opentelemetry-sdk-trace-base/src/Tracer.ts
@@ -19,8 +19,6 @@ import {
   InstrumentationLibrary,
   sanitizeAttributes,
   isTracingSuppressed,
-  AnchoredClock,
-  otperformance,
 } from '@opentelemetry/core';
 import { Resource } from '@opentelemetry/resources';
 import { BasicTracerProvider } from './BasicTracerProvider';
@@ -74,22 +72,10 @@ export class Tracer implements api.Tracer {
       context = api.trace.deleteSpan(context);
     }
     const parentSpan = api.trace.getSpan(context);
-    let clock: AnchoredClock | undefined;
-    if (parentSpan) {
-      clock = (parentSpan as any)['_clock'];
-    }
-
-    if (!clock) {
-      clock = new AnchoredClock(Date, otperformance);
-      if (parentSpan) {
-        (parentSpan as any)['_clock'] = clock;
-      }
-    }
 
     if (isTracingSuppressed(context)) {
       api.diag.debug('Instrumentation suppressed, returning Noop Span');
       const nonRecordingSpan = api.trace.wrapSpanContext(api.INVALID_SPAN_CONTEXT);
-      (nonRecordingSpan as any)['_clock'] = clock;
       return nonRecordingSpan;
     }
 
@@ -134,7 +120,6 @@ export class Tracer implements api.Tracer {
     if (samplingResult.decision === api.SamplingDecision.NOT_RECORD) {
       api.diag.debug('Recording is off, propagating context in a non-recording span');
       const nonRecordingSpan = api.trace.wrapSpanContext(spanContext);
-      (nonRecordingSpan as any)['_clock'] = clock;
       return nonRecordingSpan;
     }
 
@@ -147,7 +132,6 @@ export class Tracer implements api.Tracer {
       parentSpanId,
       links,
       options.startTime,
-      clock,
     );
     // Set initial span attributes. The attributes object may have been mutated
     // by the sampler, so we sanitize the merged attributes before setting them.