search

open-telemetry / opentelemetry-js

OpenTelemetry JavaScript Client
https://opentelemetry.io
Apache License 2.0
2.75k stars 809 forks source link

Invalid types in Node SDK? #5062

Open robcresswell opened 1 month ago

robcresswell commented 1 month ago

What happened?

Steps to Reproduce

I've got a fairly straightforward setup, I hope:

export function register() {
  const sdk = new NodeSDK({
    resource: new Resource({
      [ATTR_SERVICE_NAME]: 'service',
    }),
    instrumentations: [
      new HttpInstrumentation(),
      new FastifyInstrumentation(),
      new PgInstrumentation(),
    ],
    spanProcessors: [new SimpleSpanProcessor(new OTLPTraceExporter())],
  });
  sdk.start();
}

register();

which gives me the following type error:

Type 'SimpleSpanProcessor' is not assignable to type 'SpanProcessor'.
  Types of property 'onStart' are incompatible.
    Type '(_span: Span, _parentContext: Context) => void' is not assignable to type '(span: Span, parentContext: Context) => void'.
      Types of parameters '_span' and 'span' are incompatible.
        Type 'import("/Users/robcresswell/tessl/backend/node_modules/@opentelemetry/sdk-trace-base/build/src/Span").Span' is not assignable to type 'import("/Users/robcresswell/tessl/backend/node_modules/@opentelemetry/sdk-trace-node/node_modules/@opentelemetry/sdk-trace-base/build/src/Span").Span'.
          Types have separate declarations of a private property '_spanContext'.

OpenTelemetry Setup Code

No response

package.json

"@opentelemetry/api": "^1.9.0", "@opentelemetry/exporter-trace-otlp-grpc": "^0.53.0", "@opentelemetry/instrumentation-fastify": "^0.40.0", "@opentelemetry/instrumentation-grpc": "^0.53.0", "@opentelemetry/instrumentation-http": "^0.53.0", "@opentelemetry/instrumentation-pg": "^0.45.0", "@opentelemetry/resources": "^1.26.0", "@opentelemetry/sdk-node": "^0.53.0", "@opentelemetry/sdk-trace-node": "^1.26.0", "@opentelemetry/semantic-conventions": "^1.27.0",

Relevant log output

No response

robcresswell commented 1 month ago

Also the boxes to fill in the package.json etc seem uneditable in the form?

robcresswell commented 1 month ago

Hm, maybe my browser just bugged out

pichlermarc commented 1 month ago

@robcresswell does this also happen on a clean reinstall of the dependencies? (removed package-lock.json and node_modules). Would you mind showing us the npm ls output so we can further investigate? :thinking:

trentm commented 1 month ago

FWIW, I was not able to reproduce as follows:

package.json

{
  "name": "asdf.20241016t084622",
  "version": "1.0.0",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "keywords": [],
  "author": "",
  "license": "ISC",
  "description": "",
  "dependencies": {
    "@opentelemetry/api": "^1.9.0",
    "@opentelemetry/exporter-trace-otlp-grpc": "^0.53.0",
    "@opentelemetry/instrumentation-fastify": "^0.40.0",
    "@opentelemetry/instrumentation-grpc": "^0.53.0",
    "@opentelemetry/instrumentation-http": "^0.53.0",
    "@opentelemetry/instrumentation-pg": "^0.45.0",
    "@opentelemetry/resources": "^1.26.0",
    "@opentelemetry/sdk-node": "^0.53.0",
    "@opentelemetry/sdk-trace-node": "^1.26.0",
    "@opentelemetry/semantic-conventions": "^1.27.0"
  }
}

reg-issue-5062.mjs

import {NodeSDK} from '@opentelemetry/sdk-node';
import { SimpleSpanProcessor } from '@opentelemetry/sdk-trace-base';
import { OTLPTraceExporter } from '@opentelemetry/exporter-trace-otlp-grpc';
import {Resource} from '@opentelemetry/resources'
import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
import { FastifyInstrumentation } from '@opentelemetry/instrumentation-fastify';
import { PgInstrumentation } from '@opentelemetry/instrumentation-pg';

export function register() {
  const sdk = new NodeSDK({
    resource: new Resource({
      'service.name': 'service',
    }),
    instrumentations: [
      new HttpInstrumentation(),
      new FastifyInstrumentation(),
      new PgInstrumentation(),
    ],
    spanProcessors: [new SimpleSpanProcessor(new OTLPTraceExporter())],
  });
  sdk.start();
}

register();
% npm install
...

% npm ls
asdf.20241016t084622@1.0.0 /Users/trentm/tmp/asdf.20241016T084622
├── @opentelemetry/api@1.9.0
├── @opentelemetry/exporter-trace-otlp-grpc@0.53.0
├── @opentelemetry/instrumentation-fastify@0.40.0
├── @opentelemetry/instrumentation-grpc@0.53.0
├── @opentelemetry/instrumentation-http@0.53.0
├── @opentelemetry/instrumentation-pg@0.45.1
├── @opentelemetry/resources@1.26.0
├── @opentelemetry/sdk-node@0.53.0
├── @opentelemetry/sdk-trace-node@1.26.0
└── @opentelemetry/semantic-conventions@1.27.0

% node reg-issue-5062.mjs
%
robcresswell commented 1 month ago

I'm unable to recreate this now unfortunately, though I did try clearing node_modules, cache etc at the time. I'm not sure whats shifted in the past week, but it all seems happy now.

robcresswell commented 2 weeks ago

Okay, I've managed to recreate this.

npm ls

├── @opentelemetry/api@1.9.0
├── @opentelemetry/exporter-trace-otlp-grpc@0.54.0
├── @opentelemetry/instrumentation-fastify@0.40.0
├── @opentelemetry/instrumentation-grpc@0.53.0
├── @opentelemetry/instrumentation-http@0.54.0
├── @opentelemetry/instrumentation-pg@0.46.0
├── @opentelemetry/resources@1.27.0
├── @opentelemetry/sdk-node@0.53.0
├── @opentelemetry/sdk-trace-node@1.27.0
└── @opentelemetry/semantic-conventions@1.27.0

package.json

{
  "name": "repro",
  "version": "1.0.0",
  "main": "index.js",
  "dependencies": {
    "@opentelemetry/api": "^1.9.0",
    "@opentelemetry/exporter-trace-otlp-grpc": "^0.54.0",
    "@opentelemetry/instrumentation-fastify": "^0.40.0",
    "@opentelemetry/instrumentation-grpc": "^0.53.0",
    "@opentelemetry/instrumentation-http": "^0.54.0",
    "@opentelemetry/instrumentation-pg": "^0.46.0",
    "@opentelemetry/resources": "^1.27.0",
    "@opentelemetry/sdk-node": "^0.53.0",
    "@opentelemetry/sdk-trace-node": "^1.27.0",
    "@opentelemetry/semantic-conventions": "^1.27.0"
  }
}

tsconfig.json

{
  "compilerOptions": {
    "outDir": "./dist",
    "target": "ES2022",
    "lib": ["ES2023"],
    "module": "NodeNext",
    "moduleResolution": "NodeNext",
    "strict": true,
  },
  "include": ["index.ts"],
}

index.ts

import { NodeSDK } from '@opentelemetry/sdk-node';
import { SimpleSpanProcessor } from '@opentelemetry/sdk-trace-base';
import { OTLPTraceExporter } from '@opentelemetry/exporter-trace-otlp-grpc';
import { Resource } from '@opentelemetry/resources'
import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
import { FastifyInstrumentation } from '@opentelemetry/instrumentation-fastify';
import { PgInstrumentation } from '@opentelemetry/instrumentation-pg';

export function register() {
  const sdk = new NodeSDK({
    resource: new Resource({
      'service.name': 'service',
    }),
    instrumentations: [
      new HttpInstrumentation(),
      new FastifyInstrumentation(),
      new PgInstrumentation(),
    ],
    spanProcessors: [new SimpleSpanProcessor(new OTLPTraceExporter())],
  });
  sdk.start();
}

register();
david-luna commented 2 weeks ago

@robcresswell

The TS error mentions that the Span types are taken from different paths. So if check for @opentelemetry/sdk-trace-base.

npm ls @opentelemetry/sdk-trace-base

repro@1.0.0 /Users/david/Documents/issues/otel/5062-core
├─┬ @opentelemetry/exporter-trace-otlp-grpc@0.54.0
│ ├─┬ @opentelemetry/otlp-transformer@0.54.0
│ │ └── @opentelemetry/sdk-trace-base@1.27.0 deduped
│ └── @opentelemetry/sdk-trace-base@1.27.0
├─┬ @opentelemetry/sdk-node@0.53.0
│ ├─┬ @opentelemetry/exporter-logs-otlp-grpc@0.53.0
│ │ └─┬ @opentelemetry/otlp-transformer@0.53.0
│ │   └── @opentelemetry/sdk-trace-base@1.26.0
│ ├─┬ @opentelemetry/exporter-logs-otlp-http@0.53.0
│ │ └─┬ @opentelemetry/otlp-transformer@0.53.0
│ │   └── @opentelemetry/sdk-trace-base@1.26.0
│ ├─┬ @opentelemetry/exporter-logs-otlp-proto@0.53.0
│ │ ├─┬ @opentelemetry/otlp-transformer@0.53.0
│ │ │ └── @opentelemetry/sdk-trace-base@1.26.0 deduped
│ │ └── @opentelemetry/sdk-trace-base@1.26.0
│ ├─┬ @opentelemetry/exporter-trace-otlp-grpc@0.53.0
│ │ ├─┬ @opentelemetry/otlp-transformer@0.53.0
│ │ │ └── @opentelemetry/sdk-trace-base@1.26.0 deduped
│ │ └── @opentelemetry/sdk-trace-base@1.26.0 deduped
│ ├─┬ @opentelemetry/exporter-trace-otlp-http@0.53.0
│ │ ├─┬ @opentelemetry/otlp-transformer@0.53.0
│ │ │ └── @opentelemetry/sdk-trace-base@1.26.0 deduped
│ │ └── @opentelemetry/sdk-trace-base@1.26.0
│ ├─┬ @opentelemetry/exporter-trace-otlp-proto@0.53.0
│ │ ├─┬ @opentelemetry/otlp-transformer@0.53.0
│ │ │ └── @opentelemetry/sdk-trace-base@1.26.0 deduped
│ │ └── @opentelemetry/sdk-trace-base@1.26.0
│ ├─┬ @opentelemetry/exporter-zipkin@1.26.0
│ │ └── @opentelemetry/sdk-trace-base@1.26.0
│ ├── @opentelemetry/sdk-trace-base@1.26.0
│ └─┬ @opentelemetry/sdk-trace-node@1.26.0
│   └── @opentelemetry/sdk-trace-base@1.26.0 deduped
└─┬ @opentelemetry/sdk-trace-node@1.27.0
  └── @opentelemetry/sdk-trace-base@1.27.0 deduped

We see that

is not optimal but I think it shouldn't be an issue. A quick workaround would be to remove @opentelemetry/sdk-trace-base and @opentelemetry/exporter-trace-otlp-grpc from the package since they are direct dependencies of @opentelemetry/sdk-node

I'll continue the investigation a bit more to see if there is a way to make it work.

david-luna commented 2 weeks ago

Apparently TS doesn't like identical type declarations from different places if they contain private properties. I've created a repository which reproduces the issue. In the first commit the same error is raised by TS. The second commit makes the error go away.

As per my previous comment this happens because there are 2 versions of @opentelemetry/sdk-trace-base generating the tree displayed in the previous comment:

TS resolves the import to the package installed at the top (1.27.0) but SDK constructor expects the one installed within its dependencies. Both type declarations are in different paths and, although they are compatible, the error appears.

A quick workaround would be to remove @opentelemetry/sdk-trace-base and @opentelemetry/exporter-trace-otlp-grpc from the package since they are direct dependencies of @opentelemetry/sdk-node

I guess the majority of users just want to setup the instrumentation so I think this not a workaround but the solution to their problem. But for other uses that might be annoying. If for some reason I need to install @opentelemetry/sdk-trace-base or other packages that have @opentelemetry/sdk-trace-base as a dependency I'll face this issue.

Should we allow users to have different versions installed? 🤔

@open-telemetry/javascript-maintainers thoughts?

pichlermarc commented 1 week ago

Should we allow users to have different versions installed? 🤔

Usually that's not supported (yet).

That's actually one of the big things that we're trying to solve with SDK 2.0 by (ideally) not exporting any classes directly, but using interfaces instead (that's one of the reasons for #3597, my proposal at https://github.com/open-telemetry/opentelemetry-js/pull/4932).

Interfaces don't have that problem as we cant declare private properties on them. So even if they come from different package versions IF we adhere to semver and only add optional properties.