fix: improve baggage implementation

index.d.ts

@@ -161,18 +161,54 @@ interface Tracer extends opentracing.Tracer {
 
   /**
    * @experimental
+   *
+   * Set a baggage item and return the new context.
+   *
+   * @see https://opentelemetry.io/docs/specs/otel/baggage/api/#set-value
+   *
+   * ----
+   *
    * Provide same functionality as OpenTelemetry Baggage:
    * https://opentelemetry.io/docs/concepts/signals/baggage/
    *
    * Since the equivalent of OTel Context is implicit in dd-trace-js,
    * these APIs act on the currently active baggage
    *
-   * Work with storage('baggage'), therefore do not follow the same continuity as other APIs
+   * Work with storage('baggage'), therefore do not follow the same continuity as other APIs.
+   */
+  setBaggageItem (key: string, value: string, metadata?: object): Record<string, string>;
+  /**
+   * @experimental
+   *
+   * Returns a specific baggage item from the current context.
+   *
+   * @see https://opentelemetry.io/docs/specs/otel/baggage/api/#get-value
    */
-  setBaggageItem (key: string, value: string): Record<string, string>;
   getBaggageItem (key: string): string | undefined;
+  /**
+   * @experimental
+   *
+   * Returns all baggage items from the current context.
+   *
+   * @see https://opentelemetry.io/docs/specs/otel/baggage/api/#get-all-values
+   */
   getAllBaggageItems (): Record<string, string>;
+  /**
+   * @experimental
+   *
+   * Removes a specific baggage item from the current context and returns the new context.
+   *
+   * @see https://opentelemetry.io/docs/specs/otel/baggage/api/#remove-value
+   */
   removeBaggageItem (key: string): Record<string, string>;
+
+  /**
+   * @experimental
+   *
+   * Removes all baggage items from the current context and returns the new context.
+   *
+   * @see https://opentelemetry.io/docs/specs/otel/baggage/api/#remove-all-values
+   */
   removeAllBaggageItems (): Record<string, string>;
 }
 

packages/datadog-core/src/storage.js

@@ -10,12 +10,12 @@ const { AsyncLocalStorage } = require('async_hooks')
  * a "handle" object, which is used as a key in a WeakMap, where the values
  * are the real store objects.
  *
- * @typedef {Record<string, unknown>} Store
+ * @template T
+ * @typedef {Record<string, T>} Store
  */
 class DatadogStorage extends AsyncLocalStorage {
   /**
-   *
-   * @param {Store} [store]
+   * @param {Store<unknown>} [store]
    * @override
    */
   enterWith (store) {
@@ -35,7 +35,7 @@ class DatadogStorage extends AsyncLocalStorage {
    *
    * TODO: Refactor the Scope class to use a span-only store and remove this.
    *
-   * @returns {Store}
+   * @returns {Store<unknown>}
    */
   getHandle () {
     return super.getStore()
@@ -48,7 +48,7 @@ class DatadogStorage extends AsyncLocalStorage {
    * key. This is useful if you've stashed a handle somewhere and want to
    * retrieve the store with it.
    * @param {object} [handle]
-   * @returns {Store | undefined}
+   * @returns {Store<unknown> | undefined}
    * @override
    */
   getStore (handle) {
@@ -68,7 +68,7 @@ class DatadogStorage extends AsyncLocalStorage {
    * WeakMap.
    * @template R
    * @template TArgs = unknown[]
-   * @param {Store} store
+   * @param {Store<unknown>} store
    * @param {() => R} fn
    * @param {...TArgs} args
    * @returns {R}
@@ -87,7 +87,7 @@ class DatadogStorage extends AsyncLocalStorage {
 
 /**
  * This is the map from handles to real stores, used in the class above.
- * @type {WeakMap<WeakKey, Store|undefined>}
+ * @type {WeakMap<WeakKey, Store<unknown>|undefined>}
  */
 const stores = new WeakMap()
 

packages/dd-trace/src/baggage.js

@@ -1,41 +1,66 @@
 'use strict'
 
 const { storage } = require('../../datadog-core')
-const baggageStorage = storage('baggage')
 
+/**
+ * Spec (API semantics):
+ * - OpenTelemetry Baggage API: https://opentelemetry.io/docs/specs/otel/baggage/api/
+ *
+ * In-process baggage is a string->string map stored in async local storage.
+ * @typedef {import('../../datadog-core/src/storage').Store<string>} BaggageStore
+ */
+
+/**
+ * @type {{ enterWith: (store?: BaggageStore) => void, getStore: () => (BaggageStore | undefined) }}
+ */
+const baggageStorage =
+  /** @type {{ enterWith: (store?: BaggageStore) => void, getStore: () => (BaggageStore | undefined) }} */ (
+    /** @type {unknown} */ (storage('baggage'))
+  )
+
+// TODO: Implement metadata https://opentelemetry.io/docs/specs/otel/baggage/api/#set-value
 /**
  * @param {string} key
  * @param {string} value
+ * @param {object} [metadata] Not used yet
  */
-function setBaggageItem (key, value) {
-  storage('baggage').enterWith({ ...baggageStorage.getStore(), [key]: value })
-  return storage('baggage').getStore()
+function setBaggageItem (key, value, metadata) {
+  if (typeof key !== 'string' || typeof value !== 'string' || key === '') {
+    return baggageStorage.getStore() ?? {}
+  }
+
+  const store = baggageStorage.getStore()
+  const newStore = { ...store, [key]: value }
+  baggageStorage.enterWith(newStore)
+  return newStore
 }
 
 /**
  * @param {string} key
+ * @returns {string | undefined}
  */
 function getBaggageItem (key) {
-  return storage('baggage').getStore()?.[key]
+  return baggageStorage.getStore()?.[key]
 }
 
 function getAllBaggageItems () {
-  return storage('baggage').getStore() ?? {}
+  return baggageStorage.getStore() ?? {}
 }
 
 /**
  * @param {string} keyToRemove
- * @returns {Record<string, unknown>}
  */
 function removeBaggageItem (keyToRemove) {
-  const { [keyToRemove]: _, ...newBaggage } = storage('baggage').getStore()
-  storage('baggage').enterWith(newBaggage)
+  const store = baggageStorage.getStore() ?? {}
+  const { [keyToRemove]: _, ...newBaggage } = store
+  baggageStorage.enterWith(newBaggage)
   return newBaggage
 }
 
 function removeAllBaggageItems () {
-  storage('baggage').enterWith()
-  return storage('baggage').getStore()
+  const newContext = /** @type {BaggageStore} */ ({})
+  baggageStorage.enterWith(newContext)
+  return newContext
 }
 
 module.exports = {

packages/dd-trace/src/opentracing/propagation/text_map.js

@@ -34,6 +34,11 @@ const b3HeaderKey = 'b3'
 const sqsdHeaderHey = 'x-aws-sqsd-attr-_datadog'
 const b3HeaderExpr = /^(([0-9a-f]{16}){1,2}-[0-9a-f]{16}(-[01d](-[0-9a-f]{16})?)?|[01d])$/i
 const baggageExpr = new RegExp(`^${baggagePrefix}(.+)$`)
+// W3C Baggage key grammar: key = token (RFC 7230).
+// Spec (up-to-date): "Propagation format for distributed context: Baggage" §3.3.1
+// https://www.w3.org/TR/baggage/#header-content
+// https://www.rfc-editor.org/rfc/rfc7230#section-3.2.6
+const baggageTokenExpr = /^[!#$%&'*+\-.^_`|~0-9A-Za-z]+$/
 const tagKeyExpr = /^_dd\.p\.[\x21-\x2B\x2D-\x7E]+$/ // ASCII minus spaces and commas
 const tagValueExpr = /^[\x20-\x2B\x2D-\x7E]*$/ // ASCII minus commas
 // RFC7230 token (used by HTTP header field-name) and compatible with Node's header name validation.
@@ -124,13 +129,6 @@ class TextMapPropagator {
     }
   }
 
-  _encodeOtelBaggageKey (key) {
-    let encoded = encodeURIComponent(key)
-    encoded = encoded.replaceAll('(', '%28')
-    encoded = encoded.replaceAll(')', '%29')
-    return encoded
-  }
-
   _injectBaggageItems (spanContext, carrier) {
     if (this._config.legacyBaggageEnabled) {
       const baggageItems = spanContext?._baggageItems
@@ -157,7 +155,13 @@ class TextMapPropagator {
       const baggageItems = getAllBaggageItems()
       if (!baggageItems) return
       for (const [key, value] of Object.entries(baggageItems)) {
-        const item = `${this._encodeOtelBaggageKey(String(key).trim())}=${encodeURIComponent(String(value).trim())},`
+        const baggageKey = String(key).trim()
+        if (!baggageKey || !baggageTokenExpr.test(baggageKey)) continue
+
+        // Do not trim values. If callers include leading/trailing whitespace, it must be percent-encoded.
+        // W3C list-member allows optional properties after ';'.
+        // https://www.w3.org/TR/baggage/#header-content
+        const item = `${baggageKey}=${encodeURIComponent(String(value))},`
         itemCounter += 1
         byteCounter += Buffer.byteLength(item)
 
@@ -640,13 +644,6 @@ class TextMapPropagator {
     }
   }
 
-  _decodeOtelBaggageKey (key) {
-    let decoded = decodeURIComponent(key)
-    decoded = decoded.replaceAll('%28', '(')
-    decoded = decoded.replaceAll('%29', ')')
-    return decoded
-  }
-
   _extractLegacyBaggageItems (carrier, spanContext) {
     if (this._config.legacyBaggageEnabled) {
       Object.keys(carrier).forEach(key => {
@@ -668,19 +665,38 @@ class TextMapPropagator {
       ? undefined
       : new Set(this._config.baggageTagKeys.split(','))
     for (const keyValue of baggages) {
-      if (!keyValue.includes('=')) {
+      if (!keyValue) continue
+
+      // Per W3C baggage, list-members can contain optional properties after `;`.
+      // Example: key=value;prop=1;prop2
+      // https://www.w3.org/TR/baggage/#header-content
+      const semicolonIdx = keyValue.indexOf(';')
+      const member = (semicolonIdx === -1 ? keyValue : keyValue.slice(0, semicolonIdx)).trim()
+      if (!member) continue
+
+      const eqIdx = member.indexOf('=')
+      if (eqIdx === -1) {
         tracerMetrics.count('context_header_style.malformed', ['header_style:baggage']).inc()
         removeAllBaggageItems()
         return
       }
-      let [key, value] = keyValue.split('=')
-      key = this._decodeOtelBaggageKey(key.trim())
-      value = decodeURIComponent(value.trim())
-      if (!key || !value) {
+
+      const key = member.slice(0, eqIdx).trim()
+      let value = member.slice(eqIdx + 1).trim()
+
+      if (!baggageTokenExpr.test(key) || !value) {
         tracerMetrics.count('context_header_style.malformed', ['header_style:baggage']).inc()
         removeAllBaggageItems()
         return
       }
+      try {
+        value = decodeURIComponent(value)
+      } catch {
+        tracerMetrics.count('context_header_style.malformed', ['header_style:baggage']).inc()
+        removeAllBaggageItems()
+        return
+      }
+
       if (spanContext && (tagAllKeys || keysToSpanTag?.has(key))) {
         spanContext._trace.tags['baggage.' + key] = value
       }

packages/dd-trace/test/opentracing/propagation/text_map.spec.js

@@ -76,6 +76,8 @@ describe('TextMapPropagator', () => {
 
   describe('inject', () => {
     beforeEach(() => {
+      removeAllBaggageItems()
+
       baggageItems = {
         foo: 'bar'
       }
@@ -147,10 +149,22 @@ describe('TextMapPropagator', () => {
 
     it('should handle special characters in baggage', () => {
       const carrier = {}
-      setBaggageItem('",;\\()/:<=>?@[]{}🐶é我', '",;\\🐶é我')
+      // W3C baggage keys must be RFC7230 tokens; keep special chars in the value.
+      setBaggageItem('special', '",;\\🐶é我')
       propagator.inject(undefined, carrier)
-      // eslint-disable-next-line @stylistic/max-len
-      assert.strictEqual(carrier.baggage, '%22%2C%3B%5C%28%29%2F%3A%3C%3D%3E%3F%40%5B%5D%7B%7D%F0%9F%90%B6%C3%A9%E6%88%91=%22%2C%3B%5C%F0%9F%90%B6%C3%A9%E6%88%91')
+      assert.strictEqual(carrier.baggage, 'special=%22%2C%3B%5C%F0%9F%90%B6%C3%A9%E6%88%91')
+    })
+
+    it('should not accept special characters in baggage key', () => {
+      const tracerMetrics = telemetryMetrics.manager.namespace('tracers')
+
+      const carrier = {}
+      // W3C baggage keys must be RFC7230 tokens; keep special chars in the value.
+      setBaggageItem('",;\\()/:<=>?@[]{}🐶é我', 'test value')
+      propagator.inject(undefined, carrier)
+      assert.strictEqual(carrier.baggage, undefined)
+
+      sinon.assert.notCalled(tracerMetrics.count)
     })
 
     it('should drop excess baggage items when there are too many pairs', () => {
@@ -545,11 +559,11 @@ describe('TextMapPropagator', () => {
       const carrier = {
         'x-datadog-trace-id': '123',
         'x-datadog-parent-id': '456',
-        baggage: '%22%2C%3B%5C%28%29%2F%3A%3C%3D%3E%3F%40%5B%5D%7B%7D=%22%2C%3B%5C'
+        baggage: 'special=%22%2C%3B%5C'
       }
       const spanContext = propagator.extract(carrier)
       assert.deepStrictEqual(spanContext._baggageItems, {})
-      assert.deepStrictEqual(getAllBaggageItems(), { '",;\\()/:<=>?@[]{}': '",;\\' })
+      assert.deepStrictEqual(getAllBaggageItems(), { special: '",;\\' })
     })
 
     it('should not extract baggage when the header is malformed', () => {
@@ -590,6 +604,17 @@ describe('TextMapPropagator', () => {
       assert.deepStrictEqual(getAllBaggageItems(), {})
     })
 
+    it('should not extract metadata properties from baggage', () => {
+      const carrier = {
+        'x-datadog-trace-id': '123',
+        'x-datadog-parent-id': '456',
+        baggage: 'name=  test value;prop=1; prop2=2'
+      }
+      const spanContext = propagator.extract(carrier)
+      assert.deepStrictEqual(spanContext._baggageItems, {})
+      assert.deepStrictEqual(getAllBaggageItems(), { name: 'test value' })
+    })
+
     it('should add baggage items to span tags', () => {
       // should add baggage with default keys
       let carrier = {

packages/dd-trace/test/proxy.spec.js

@@ -716,6 +716,19 @@ describe('TracerProxy', () => {
           const baggage = proxy.setBaggageItem('key2', 'value2')
           assert.deepStrictEqual(baggage, { key1: 'value1', key2: 'value2' })
         })
+
+        it('should ignore invalid key or value', () => {
+          proxy.setBaggageItem(null, 'value')
+          proxy.setBaggageItem(123, 'value')
+
+          // Valid
+          proxy.setBaggageItem('key1', 'value1')
+
+          proxy.setBaggageItem('key2', 333)
+          const baggage = proxy.setBaggageItem('key3', {})
+
+          assert.deepStrictEqual(baggage, { key1: 'value1' })
+        })
       })
 
       describe('getBaggageItem', () => {
@@ -761,7 +774,7 @@ describe('TracerProxy', () => {
           proxy.setBaggageItem('key1', 'value1')
           proxy.setBaggageItem('key2', 'value2')
           const baggage = proxy.removeAllBaggageItems()
-          assert.strictEqual(baggage, undefined)
+          assert.deepStrictEqual(baggage, {})
         })
       })
     })