Merge branch 'main' into dluna-ci-pr-speed-and-coverage

Author: david-luna

plugins/node/instrumentation-undici/src/types.ts

@@ -22,9 +22,10 @@ export interface UndiciRequest {
   path: string;
   /**
    * Serialized string of headers in the form `name: value\r\n` for v5
-   * Array of strings v6
+   * Array of strings `[key1, value1, key2, value2]`, where values are
+   * `string | string[]` for v6
    */
-  headers: string | string[];
+  headers: string | (string | string[])[];
   /**
    * Helper method to add headers (from v6)
    */

plugins/node/instrumentation-undici/src/undici.ts

@@ -165,6 +165,50 @@ export class UndiciInstrumentation extends InstrumentationBase<UndiciInstrumenta
     });
   }
 
+  private parseRequestHeaders(request: UndiciRequest) {
+    const result = new Map<string, string | string[]>();
+
+    if (Array.isArray(request.headers)) {
+      // headers are an array [k1, v2, k2, v2] (undici v6+)
+      // values could be string or a string[] for multiple values
+      for (let i = 0; i < request.headers.length; i += 2) {
+        const key = request.headers[i];
+        const value = request.headers[i + 1];
+
+        // Key should always be a string, but the types don't know that, and let's be safe
+        if (typeof key === 'string') {
+          result.set(key.toLowerCase(), value);
+        }
+      }
+    } else if (typeof request.headers === 'string') {
+      // headers are a raw string (undici v5)
+      // headers could be repeated in several lines for multiple values
+      const headers = request.headers.split('\r\n');
+      for (const line of headers) {
+        if (!line) {
+          continue;
+        }
+        const colonIndex = line.indexOf(':');
+        if (colonIndex === -1) {
+          // Invalid header? Probably this can't happen, but again let's be safe.
+          continue;
+        }
+        const key = line.substring(0, colonIndex).toLowerCase();
+        const value = line.substring(colonIndex + 1).trim();
+        const allValues = result.get(key);
+
+        if (allValues && Array.isArray(allValues)) {
+          allValues.push(value);
+        } else if (allValues) {
+          result.set(key, [allValues, value]);
+        } else {
+          result.set(key, value);
+        }
+      }
+    }
+    return result;
+  }
+
   // This is the 1st message we receive for each request (fired after request creation). Here we will
   // create the span and populate some atttributes, then link the span to the request for further
   // span processing
@@ -218,24 +262,16 @@ export class UndiciInstrumentation extends InstrumentationBase<UndiciInstrumenta
     }
 
     // Get user agent from headers
-    let userAgent;
-    if (Array.isArray(request.headers)) {
-      const idx = request.headers.findIndex(
-        h => h.toLowerCase() === 'user-agent'
-      );
-      if (idx >= 0) {
-        userAgent = request.headers[idx + 1];
-      }
-    } else if (typeof request.headers === 'string') {
-      const headers = request.headers.split('\r\n');
-      const uaHeader = headers.find(h =>
-        h.toLowerCase().startsWith('user-agent')
-      );
-      userAgent =
-        uaHeader && uaHeader.substring(uaHeader.indexOf(':') + 1).trim();
-    }
-
-    if (userAgent) {
+    const headersMap = this.parseRequestHeaders(request);
+    const userAgentValues = headersMap.get('user-agent');
+
+    if (userAgentValues) {
+      // NOTE: having multiple user agents is not expected so
+      // we're going to take last one like `curl` does
+      // ref: https://curl.se/docs/manpage.html#-A
+      const userAgent = Array.isArray(userAgentValues)
+        ? userAgentValues[userAgentValues.length - 1]
+        : userAgentValues;
       attributes[SemanticAttributes.USER_AGENT_ORIGINAL] = userAgent;
     }
 
@@ -329,27 +365,14 @@ export class UndiciInstrumentation extends InstrumentationBase<UndiciInstrumenta
       const headersToAttribs = new Set(
         config.headersToSpanAttributes.requestHeaders.map(n => n.toLowerCase())
       );
+      const headersMap = this.parseRequestHeaders(request);
 
-      // headers could be in form
-      // ['name: value', ...] for v5
-      // ['name', 'value', ...] for v6
-      const rawHeaders = Array.isArray(request.headers)
-        ? request.headers
-        : request.headers.split('\r\n');
-      rawHeaders.forEach((h, idx) => {
-        const sepIndex = h.indexOf(':');
-        const hasSeparator = sepIndex !== -1;
-        const name = (
-          hasSeparator ? h.substring(0, sepIndex) : h
-        ).toLowerCase();
-        const value = hasSeparator
-          ? h.substring(sepIndex + 1)
-          : rawHeaders[idx + 1];
-
+      for (const [name, value] of headersMap.entries()) {
         if (headersToAttribs.has(name)) {
-          spanAttributes[`http.request.header.${name}`] = value.trim();
+          const attrValue = Array.isArray(value) ? value.join(', ') : value;
+          spanAttributes[`http.request.header.${name}`] = attrValue;
         }
-      });
+      }
     }
 
     span.setAttributes(spanAttributes);

plugins/node/instrumentation-undici/test/undici.test.ts

@@ -797,43 +797,87 @@ describe('UndiciInstrumentation `undici` tests', function () {
       });
     });
 
-    it('should not report an user-agent if it was not defined', async function () {
-      let spans = memoryExporter.getFinishedSpans();
-      assert.strictEqual(spans.length, 0);
+    const userAgentRequests: Array<{
+      name: string;
+      headers: Record<string, string | string[] | undefined>;
+      expectedUserAgent: string | undefined;
+    }> = [
+      {
+        name: 'no user-agent',
+        headers: { 'foo-client': 'bar' },
+        expectedUserAgent: undefined,
+      },
+      {
+        name: 'a user-agent',
+        headers: { 'foo-client': 'bar', 'user-agent': 'custom' },
+        expectedUserAgent: 'custom',
+      },
+      {
+        name: 'explicitly-undefined user-agent',
+        headers: { 'foo-client': 'bar', 'user-agent': undefined },
+        expectedUserAgent: undefined,
+      },
+      // contra the spec, but we shouldn't crash
+      {
+        name: 'multiple user-agents',
+        headers: {
+          'foo-client': 'bar',
+          'user-agent': ['agent', 'other-agent'],
+        },
+        expectedUserAgent: 'other-agent',
+      },
+      {
+        name: 'another header with value user-agent',
+        headers: { 'foo-client': 'user-agent', 'user-agent': 'custom' },
+        expectedUserAgent: 'custom',
+      },
+      {
+        name: 'another header with multiple values',
+        headers: { 'foo-client': ['one', 'two'], 'user-agent': 'custom' },
+        expectedUserAgent: 'custom',
+      },
+      {
+        name: 'another header with explicitly-undefined value',
+        headers: { 'foo-client': undefined, 'user-agent': 'custom' },
+        expectedUserAgent: 'custom',
+      },
+    ];
 
-      // Do some requests
-      const headers = {
-        'foo-client': 'bar',
-      };
+    for (const testCase of userAgentRequests) {
+      it(`should report the correct user-agent when the request has ${testCase.name}`, async function () {
+        let spans = memoryExporter.getFinishedSpans();
+        assert.strictEqual(spans.length, 0);
 
-      const queryRequestUrl = `${protocol}://${hostname}:${mockServer.port}/?query=test`;
-      const queryResponse = await undici.request(queryRequestUrl, { headers });
-      await consumeResponseBody(queryResponse.body);
+        const queryRequestUrl = `${protocol}://${hostname}:${mockServer.port}/?query=test`;
+        const queryResponse = await undici.request(queryRequestUrl, {
+          headers: testCase.headers,
+        });
+        await consumeResponseBody(queryResponse.body);
 
-      assert.ok(
-        queryResponse.headers['propagation-error'] == null,
-        'propagation is set for instrumented requests'
-      );
+        assert.ok(
+          queryResponse.headers['propagation-error'] == null,
+          'propagation is set for instrumented requests'
+        );
 
-      spans = memoryExporter.getFinishedSpans();
-      const span = spans[0];
-      assert.ok(span, 'a span is present');
-      assert.strictEqual(spans.length, 1);
-      assertSpan(span, {
-        hostname: 'localhost',
-        httpStatusCode: queryResponse.statusCode,
-        httpMethod: 'GET',
-        path: '/',
-        query: '?query=test',
-        reqHeaders: headers,
-        resHeaders: queryResponse.headers,
+        spans = memoryExporter.getFinishedSpans();
+        const span = spans[0];
+        assert.ok(span, 'a span is present');
+        assert.strictEqual(spans.length, 1);
+        assertSpan(span, {
+          hostname: 'localhost',
+          httpStatusCode: queryResponse.statusCode,
+          httpMethod: 'GET',
+          path: '/',
+          query: '?query=test',
+          reqHeaders: testCase.headers,
+          resHeaders: queryResponse.headers,
+        });
+        assert.strictEqual(
+          span.attributes['user_agent.original'],
+          testCase.expectedUserAgent
+        );
       });
-      assert.strictEqual(
-        span.attributes['user_agent.original'],
-        undefined,
-        'user-agent is undefined'
-      );
-    });
+    }
 
     it('should create valid span if request.path is a full URL', async function () {
       let spans = memoryExporter.getFinishedSpans();

plugins/node/instrumentation-undici/test/utils/assertSpan.ts

@@ -171,7 +171,7 @@ export const assertSpan = (
     if (userAgent) {
       assert.strictEqual(
         span.attributes[SemanticAttributes.USER_AGENT_ORIGINAL],
-        userAgent
+        Array.isArray(userAgent) ? userAgent[userAgent.length - 1] : userAgent
       );
     }
   }

plugins/node/opentelemetry-instrumentation-hapi/README.md

@@ -60,7 +60,7 @@ hapiInstrumentation.setTracerProvider(provider);
 
 See [examples/hapi](https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/examples/hapi) for a short example using Hapi
 
-<!-- 
+<!--
 The dev dependency of `@hapi/[email protected]` is required to force the compatible type declarations. See: https://github.com/hapijs/hapi/issues/4240
 -->
 
@@ -70,14 +70,24 @@ This package provides automatic tracing for hapi server routes and [request life
 
 ## Semantic Conventions
 
-This package uses `@opentelemetry/semantic-conventions` version `1.22+`, which implements Semantic Convention [Version 1.7.0](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/semantic_conventions/README.md)
+Prior to version `0.48.0`, this instrumentation created spans targeting an experimental semantic convention [Version 1.7.0](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/semantic_conventions/README.md).
 
-Attributes collected:
+HTTP semantic conventions (semconv) were stabilized in v1.23.0, and a [migration process](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/non-normative/http-migration.md#http-semantic-convention-stability-migration) was defined. `instrumentation-hapi` versions 0.48.0 and later include support for migrating to stable HTTP semantic conventions, as described below. The intent is to provide an approximate 6 month time window for users of this instrumentation to migrate to the new HTTP semconv, after which a new minor version will use the *new* semconv by default and drop support for the old semconv. See the [HTTP semconv migration plan for OpenTelemetry JS instrumentations](https://github.com/open-telemetry/opentelemetry-js/issues/5646).
 
-| Attribute           | Short Description                                  |
-|---------------------|----------------------------------------------------|
-| `http.method`       | HTTP method                                        |
-| `http.route`        | Route assigned to handler. Ex: `/users/:id`        |
+To select which semconv version(s) is emitted from this instrumentation, use the `OTEL_SEMCONV_STABILITY_OPT_IN` environment variable.
+
+- `http`: emit the new (stable) v1.23.0+ semantics
+- `http/dup`: emit **both** the old v1.7.0 and the new (stable) v1.23.0+ semantics
+- By default, if `OTEL_SEMCONV_STABILITY_OPT_IN` includes neither of the above tokens, the old v1.7.0 semconv is used.
+
+### Attributes collected
+
+The following semconv attributes are collected on hapi route spans:
+
+| v1.7.0 semconv | v1.23.0 semconv       | Notes |
+| -------------- | --------------------- | ----- |
+| `http.method`  | `http.request.method` | HTTP request method |
+| `http.route`   | `http.route` (same)   | Route assigned to handler. Ex: `/users/:id` |
 
 ## Useful links
 

plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts

@@ -21,6 +21,8 @@ import {
   InstrumentationConfig,
   InstrumentationNodeModuleDefinition,
   isWrapped,
+  SemconvStability,
+  semconvStabilityFromStr,
 } from '@opentelemetry/instrumentation';
 
 import type * as Hapi from '@hapi/hapi';
@@ -50,8 +52,14 @@ import {
 
 /** Hapi instrumentation for OpenTelemetry */
 export class HapiInstrumentation extends InstrumentationBase {
+  private _semconvStability: SemconvStability;
+
   constructor(config: InstrumentationConfig = {}) {
     super(PACKAGE_NAME, PACKAGE_VERSION, config);
+    this._semconvStability = semconvStabilityFromStr(
+      'http',
+      process.env.OTEL_SEMCONV_STABILITY_OPT_IN
+    );
   }
 
   protected init() {
@@ -387,7 +395,11 @@ export class HapiInstrumentation extends InstrumentationBase {
         if (rpcMetadata?.type === RPCType.HTTP) {
           rpcMetadata.route = route.path;
         }
-        const metadata = getRouteMetadata(route, pluginName);
+        const metadata = getRouteMetadata(
+          route,
+          instrumentation._semconvStability,
+          pluginName
+        );
         const span = instrumentation.tracer.startSpan(metadata.name, {
           attributes: metadata.attributes,
         });

plugins/node/opentelemetry-instrumentation-hapi/src/semconv.ts

@@ -0,0 +1,34 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/*
+ * This file contains a copy of unstable semantic convention definitions
+ * used by this package.
+ * @see https://github.com/open-telemetry/opentelemetry-js/tree/main/semantic-conventions#unstable-semconv
+ */
+
+/**
+ * Deprecated, use `http.request.method` instead.
+ *
+ * @example GET
+ * @example POST
+ * @example HEAD
+ *
+ * @experimental This attribute is experimental and is subject to breaking changes in minor releases of `@opentelemetry/semantic-conventions`.
+ *
+ * @deprecated Replaced by `http.request.method`.
+ */
+export const ATTR_HTTP_METHOD = 'http.method' as const;