Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NodeSDK fails in strict mode with Legacy octal escape is not permitted in strict mode #3759

Closed
jankaifer opened this issue Apr 25, 2023 · 15 comments · Fixed by #4049
Closed
Labels
bug Something isn't working pkg:exporter-jaeger pkg:sdk-node priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc up-for-grabs Good for taking. Extra help will be provided by maintainers

Comments

@jankaifer
Copy link

What happened?

Steps to Reproduce

Import NodeSDK in strict mode.

Expected Result

It should not fail.

Actual Result

When I run pnpm build, it fails. Logs below.

Additional Details

Reproduction: https://github.com/jankaifer/next-repro-otel-node-sdk-repro

OpenTelemetry Setup Code

import { NodeSDK } from "@opentelemetry/sdk-node";
import { SimpleSpanProcessor } from "@opentelemetry/sdk-trace-node";
import { ConsoleSpanExporter } from "@opentelemetry/sdk-trace-base";

const exporter = new ConsoleSpanExporter();

const sdk = new NodeSDK({
  spanProcessor: new SimpleSpanProcessor(exporter),
});
sdk.start();

package.json

{
  "private": true,
  "scripts": {
    "dev": "next dev",
    "build": "next build",
    "start": "next start"
  },
  "dependencies": {
    "@opentelemetry/sdk-node": "^0.38.0",
    "@opentelemetry/sdk-trace-base": "^1.12.0",
    "@opentelemetry/sdk-trace-node": "^1.12.0",
    "next": "canary",
    "react": "^18.2.0",
    "react-dom": "^18.2.0"
  },
  "devDependencies": {
    "@types/node": "^18.11.13",
    "@types/react": "^18.0.26",
    "typescript": "^4.9.4"
  }
}

Relevant log output

./node_modules/.pnpm/ansi-color@0.2.1/node_modules/ansi-color/lib/ansi-color.js
Error: 
  × Legacy octal escape is not permitted in strict mode
    ╭─[/home/pearman/dev/playground/otel-node-sdk-repro/node_modules/.pnpm/ansi-color@0.2.1/node_modules/ansi-color/lib/ansi-color.js:32:1]
 32 │   var color_attrs = color.split("+");
 33 │   var ansi_str = "";
 34 │   for(var i=0, attr; attr = color_attrs[i]; i++) {
 35 │     ansi_str += "\033[" + ANSI_CODES[attr] + "m";
    ·                  ──
 36 │   }
 37 │   ansi_str += str + "\033[" + ANSI_CODES["off"] + "m";
 38 │   return ansi_str;
    ╰────

  × Legacy octal escape is not permitted in strict mode
    ╭─[/home/pearman/dev/playground/otel-node-sdk-repro/node_modules/.pnpm/ansi-color@0.2.1/node_modules/ansi-color/lib/ansi-color.js:34:1]
 34 │   for(var i=0, attr; attr = color_attrs[i]; i++) {
 35 │     ansi_str += "\033[" + ANSI_CODES[attr] + "m";
 36 │   }
 37 │   ansi_str += str + "\033[" + ANSI_CODES["off"] + "m";
    ·                      ──
 38 │   return ansi_str;
 39 │ };
    ╰────

Caused by:
    Syntax Error

Import trace for requested module:
./node_modules/.pnpm/ansi-color@0.2.1/node_modules/ansi-color/lib/ansi-color.js
./node_modules/.pnpm/bufrw@1.3.0/node_modules/bufrw/annotated_buffer.js
./node_modules/.pnpm/bufrw@1.3.0/node_modules/bufrw/interface.js
./node_modules/.pnpm/bufrw@1.3.0/node_modules/bufrw/index.js
./node_modules/.pnpm/thriftrw@3.12.0/node_modules/thriftrw/index.js
./node_modules/.pnpm/jaeger-client@3.19.0/node_modules/jaeger-client/dist/src/reporters/udp_sender.js
./node_modules/.pnpm/@opentelemetry+exporter-jaeger@1.12.0_@opentelemetry+api@1.4.1/node_modules/@opentelemetry/exporter-jaeger/build/src/types.js
./node_modules/.pnpm/@opentelemetry+exporter-jaeger@1.12.0_@opentelemetry+api@1.4.1/node_modules/@opentelemetry/exporter-jaeger/build/src/jaeger.js
./node_modules/.pnpm/@opentelemetry+exporter-jaeger@1.12.0_@opentelemetry+api@1.4.1/node_modules/@opentelemetry/exporter-jaeger/build/src/index.js
./node_modules/.pnpm/@opentelemetry+sdk-node@0.38.0_@opentelemetry+api@1.4.1/node_modules/@opentelemetry/sdk-node/build/src/TracerProviderWithEnvExporter.js
./node_modules/.pnpm/@opentelemetry+sdk-node@0.38.0_@opentelemetry+api@1.4.1/node_modules/@opentelemetry/sdk-node/build/src/sdk.js
./node_modules/.pnpm/@opentelemetry+sdk-node@0.38.0_@opentelemetry+api@1.4.1/node_modules/@opentelemetry/sdk-node/build/src/index.js
./app/page.tsx
@jankaifer jankaifer added bug Something isn't working triage labels Apr 25, 2023
@jankaifer
Copy link
Author

cc @dyladan

@jankaifer
Copy link
Author

jankaifer commented Apr 25, 2023

The log makes it clear what causes the issue. The import order of packages is:

  1. @opentelemetry/sdk-node
  2. @opentelemetry/exporter-jaeger
  3. jaeger-client
  4. thriftrw
  5. bufrw
  6. ansi-color

I don't know where we can break this chain.

I'm unsure which dependency changed since I last tested @opentelemetry/sdk-node with next, but this is breaking for us, and it will force us to recommend using TraceExporter instead.

@jankaifer
Copy link
Author

jankaifer commented Apr 25, 2023

I couldn't reproduce the issue with the pages directory in next.
I'll investigate if we can make our setup less strict.

But it still would be useful to use up-to-date and maintained dependencies (ansi-color has one release 8 years ago).

@pichlermarc
Copy link
Member

@jankaifer are you using the JaegerExporter? If not then this bug will be solved by the next release (see #3739) 🙂

For users of @opentelemetry/expoter-jaeger it will still persist, but we encourage everyone to move to OTLP as Jaeger now supports it natively.

@jankaifer
Copy link
Author

No, we expect our users will use mostly OTLP.

Feel free to re-open the issue if you want to track this issue with export-jaeger, but it's solved for us. Thanks again.

@tom-sherman
Copy link

Hey @pichlermarc I'm also receiving this error with Next.js, see vercel/otel#8

I'm not using jaeger in my app code and AFAIA the @vercel/otel wrapper doesn't use this either. However even when ensuring I'm using v0.39 of the OTEL Node SDK I still get this error.

Any ideas?

I've also tried removing the Vercel wrapper and configuring OTEL directly, the code for that is below but I get the same issue.

import { trace, context } from '@opentelemetry/api';
import { NodeSDK } from '@opentelemetry/sdk-node';
import { OTLPTraceExporter } from '@opentelemetry/exporter-trace-otlp-http';
import { Resource } from '@opentelemetry/resources';
import { SemanticResourceAttributes } from '@opentelemetry/semantic-conventions';
import { SimpleSpanProcessor } from '@opentelemetry/sdk-trace-node';

const sdk = new NodeSDK({
  resource: new Resource({
    [SemanticResourceAttributes.SERVICE_NAME]: 'next-app',
  }),
  spanProcessor: new SimpleSpanProcessor(new OTLPTraceExporter()),
});

sdk.start();

export const tracer = trace.getTracer('next-app-tracer');
export { context };

@tom-sherman
Copy link

@jankaifer How did you solve this in the app directory? Or could you only get it to work in the page directoy?

@pichlermarc
Copy link
Member

pichlermarc commented May 30, 2023

@tom-sherman you're right, looks like this was not fixed by #3752 #3739 . I'm reopening this issue.

@pichlermarc pichlermarc reopened this May 30, 2023
@pichlermarc pichlermarc added priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc pkg:sdk-node pkg:exporter-jaeger up-for-grabs Good for taking. Extra help will be provided by maintainers and removed triage labels May 30, 2023
@pichlermarc
Copy link
Member

pichlermarc commented May 30, 2023

To fix this we'll have to either

@luismiramirez
Copy link
Contributor

How's progress on this?

I have the same error on a Next.js app using @opentelemetry/sdk-node v0.41.1.

Is there a way to work around this? I can also lend a hand to work on the fix. I'm available at #otel-js in Slack.

@Quickgs
Copy link

Quickgs commented Aug 11, 2023

Was blocked by this for a few weeks, eventually just bit the bullet and fixed it myself.

Forked ansi-color library and updated the octal escapes.

function setColor(str, color) {
  if (!color) return str;
  var color_attrs = color.split("+");
  var ansi_str = "";
  for (var i = 0, attr; attr = color_attrs[i]; i++) {
    ansi_str += "\x1b[" + ANSI_CODES[attr] + "m"; // \x1b is the hexadecimal representation for ESC (0o33 or \033 in octal)
  }
  ansi_str += str + "\x1b[" + ANSI_CODES["off"] + "m";
  return ansi_str;
}

Then set the following in package.json to override the reference.

"overrides": {
        "ansi-color": "<username>/commonjs-ansi-color-fixed#master"
    },

This solution requires that you have SSH for github cli setup so that NPM can access it if you have protected repositories.

@maggiepint
Copy link

@Quickgs - does this work in Next middleware on vercel? I tried doing basically exactly this and then I got a lot of upset that the node performance APIs were being called.

@Quickgs
Copy link

Quickgs commented Aug 14, 2023

@Quickgs - does this work in Next middleware on vercel? I tried doing basically exactly this and then I got a lot of upset that the node performance APIs were being called.

I am unsure, I am invoking it within the instrumentation.ts special file in next13 hosted on an AWS pod and sending it to a Honeycomb collector.

@tom-sherman
Copy link

@maggiepint I don't think instrumentation is available in the Edge runtime, it's cloudflare workers based which doesn't allow eg. Accurate timings.

@nirga
Copy link

nirga commented Oct 24, 2023

This is still happening to me with "@opentelemetry/sdk-node": "^0.44.0"

(Update: should be fixed with #4214)

anubhavchaturvedi added a commit to anubhavchaturvedi/commonjs-ansi-color that referenced this issue May 7, 2024
Fix for `Legacy octal escape is not permitted in strict mode`

Reference : open-telemetry/opentelemetry-js#3759 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:exporter-jaeger pkg:sdk-node priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc up-for-grabs Good for taking. Extra help will be provided by maintainers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants