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

Remove pkg elastic / require-in-the-middle #3813

Closed
polRk opened this issue May 17, 2023 · 16 comments
Closed

Remove pkg elastic / require-in-the-middle #3813

polRk opened this issue May 17, 2023 · 16 comments
Labels
bug Something isn't working triage

Comments

@polRk
Copy link

polRk commented May 17, 2023

What happened?

Steps to Reproduce

  1. Use @opentelemetry/[email protected]
  2. Bundle our app with esbuild
esbuild ./src/index.ts --platform=node --format=esm --bundle --sourcemap=inline --target=node18 --banner:js=import{createRequire}from\\'module\\'\\;const\\ require=createRequire\\(import.meta.url\\)\\; --outfile=./dist/server.mjs --metafile=./dist/meta.json
  1. Remove node_modules or move only bundle file to docker
  2. Try to start app

Expected Result

Works fine

Actual Result

Crash app on startup

Cannot find module 'is-core-module/package.json'

Additional Details

elastic/require-in-the-middle#71
browserify/resolve#302
browserify/resolve#297

OpenTelemetry Setup Code

"use strict";

import { commonConfig } from "@app/config";

import * as api from "@opentelemetry/api";
import { diag, DiagConsoleLogger, DiagLogLevel } from "@opentelemetry/api";
import { OTLPMetricExporter } from "@opentelemetry/exporter-metrics-otlp-http";
import { OTLPTraceExporter } from "@opentelemetry/exporter-trace-otlp-http";
import { registerInstrumentations } from "@opentelemetry/instrumentation";
import { HttpInstrumentation } from "@opentelemetry/instrumentation-http";
import { Resource } from "@opentelemetry/resources";
import { MeterProvider, PeriodicExportingMetricReader } from "@opentelemetry/sdk-metrics";
import { BatchSpanProcessor, NodeTracerProvider } from "@opentelemetry/sdk-trace-node";
import { SemanticResourceAttributes } from "@opentelemetry/semantic-conventions";

import pkg from "../package.json" assert { type: "json" };

if (commonConfig.debug) {
  diag.setLogger(new DiagConsoleLogger(), DiagLogLevel.DEBUG);
}

const resource = new Resource({
  [SemanticResourceAttributes.SERVICE_NAMESPACE]: "",
  [SemanticResourceAttributes.SERVICE_NAME]: "",
  [SemanticResourceAttributes.SERVICE_VERSION]: pkg.version,
  [SemanticResourceAttributes.DEPLOYMENT_ENVIRONMENT]: process.env["NODE_ENV"],
});

registerInstrumentations({
  instrumentations: [new HttpInstrumentation({ enabled: true })],
});

const traceExporter = new OTLPTraceExporter({ url: commonConfig.otel.traces });
const traceProvider = new NodeTracerProvider({ resource });
traceProvider.addSpanProcessor(new BatchSpanProcessor(traceExporter));
traceProvider.register();

const meterExporter = new OTLPMetricExporter({ url: commonConfig.otel.metrics });
const meterProvider = new MeterProvider({ resource: resource });
meterProvider.addMetricReader(new PeriodicExportingMetricReader({ exporter: meterExporter }));

export const tracer = api.trace.getTracer(pkg.name);
export const meter = api.metrics.getMeter(pkg.name);

api.metrics.setGlobalMeterProvider(meterProvider);

export const metrics = {
  resolveFieldTotal: meter.createCounter("graphql.resolve.field.total"),
  resolveFieldErrors: meter.createCounter("graphql.resolve.field.errors"),
  resolveFieldDuration: meter.createHistogram("graphql.resolve.field.duration"),
};

["SIGINT", "SIGTERM"].forEach((signal) => {
  process.on(signal, () => traceProvider.shutdown().catch(console.error));
  process.on(signal, () => meterProvider.shutdown().catch(console.error));
});

package.json

"@opentelemetry/api": "^1.4.0",
    "@opentelemetry/exporter-metrics-otlp-http": "0.35.1",
    "@opentelemetry/exporter-trace-otlp-http": "0.35.1",
    "@opentelemetry/instrumentation": "0.35.1",
    "@opentelemetry/instrumentation-graphql": "0.33.0",
    "@opentelemetry/instrumentation-grpc": "0.35.1",
    "@opentelemetry/instrumentation-http": "0.35.1",
    "@opentelemetry/resources": "1.9.1",
    "@opentelemetry/sdk-metrics": "1.12.0",
    "@opentelemetry/sdk-trace-node": "1.9.1",
    "@opentelemetry/semantic-conventions": "1.9.1",

Relevant log output

Cannot find module 'is-core-module/package.json'
@polRk polRk added bug Something isn't working triage labels May 17, 2023
@polRk
Copy link
Author

polRk commented May 17, 2023

File system:

index.ts
telemetry.ts

intex.ts cotains

import "./env";
import "./telemetry";

import { CONFIG } from "@app/config";
import { createYoga } from "graphql-yoga";
import { createServer as createHTTPServer } from "node:http";

(async function () {
  logger.info("⌛ Server is starting...");

  const yoga = createYoga()
  const httpServer = createHTTPServer(yoga);

  httpServer.on("error", (e) => {
    logger.error(e, "❌ Cannot start the server.");
    process.exit(1);
  });

  const [hostname, port] = CONFIG.host.split(":");
  httpServer.listen(parseInt(port!), hostname);

})();

@dyladan
Copy link
Member

dyladan commented May 17, 2023

Require in the middle is fundamental to the way most if not all of our instrumentations work and it isn't easy to just remove. I think this seems more like an issue with esbuild configuration. Maybe we can do something that improves the situation or maybe require-in-the-middle can do something? I think even if you get the application to load, opentelemetry is likely to be completely broken because we depend so heavily on the require patching provided by RITM

@polRk
Copy link
Author

polRk commented May 18, 2023

Maybe we can drop this pkg and write another one similar in @opentelemetry org? Ex:
@opentelemetry/require-in-middle?

@polRk
Copy link
Author

polRk commented May 18, 2023

I think this seems more like an issue with esbuild configuration.

No the problem is , browserfy/resolve try to load file from filesystem, but i have only 1 js file (bundle) without any node_modules.

I do not understand, why require-in-the-middle use that pkg

1 similar comment
@polRk
Copy link
Author

polRk commented May 18, 2023

I think this seems more like an issue with esbuild configuration.

No the problem is , browserfy/resolve try to load file from filesystem, but i have only 1 js file (bundle) without any node_modules.

I do not understand, why require-in-the-middle use that pkg

@dyladan
Copy link
Member

dyladan commented May 18, 2023

There are many other dependencies that are loaded properly. Why would that one fail?

@dyladan
Copy link
Member

dyladan commented May 18, 2023

I'm looking through require-in-the-middle and I don't see any use of is-core-module. Are you sure that is the one causing the issue?

@polRk
Copy link
Author

polRk commented May 18, 2023

I'm looking through require-in-the-middle and I don't see any use of is-core-module. Are you sure that is the one causing the issue?

│ └─┬ @opentelemetry/[email protected]
│   └─┬ [email protected]
│     └─┬ [email protected]
│       └── [email protected]

@trentm
Copy link
Contributor

trentm commented May 22, 2023

@polRk I am trying to understand the history a little bit. It looks like the maintainer of resolve downgraded the "latest" version from [email protected] to [email protected]. This effectively -- for now at least -- reverts browserify/resolve@v1.22.2...v1.22.3 which includes the fs.readFileSync change that is causing the issue with Webpack -- as discussed at browserify/resolve#297. Is that right?

So if you re-build cleanly so that you get [email protected], does the Webpack bundle work now? I understand that this doesn't necessarily solve the issue going forward.

Update: I said "Webpack", but I meant "esbuild".

@trentm
Copy link
Contributor

trentm commented May 22, 2023

I think even if you get the application to load, opentelemetry is likely to be completely broken because we depend so heavily on the require patching provided by RITM

I'll echo this. If it helps for some background -- and apologies for the vendor-specific docs -- https://www.elastic.co/guide/en/apm/agent/nodejs/current/starting-the-agent.html#start-bundlers explains some of the issues with attempting to use bundlers with anything that uses require-in-the-middle (including the @opentelemetry/instrumentation package).

@Flarna
Copy link
Member

Flarna commented May 22, 2023

I think even if you get the application to load, opentelemetry is likely to be completely broken because we depend so heavily on the require patching provided by RITM

Only instrumentations should be effected by removing RITM. One can still manual instrument and OTel works fine.

@trentm
Copy link
Contributor

trentm commented May 23, 2023

One can still manual instrument and OTel works fine.

Yes, fair. Thanks.

@trentm
Copy link
Contributor

trentm commented May 24, 2023

@polRk Can you confirm that things work for you now that the maintainer of the resolve module has moved the "latest" tag back to the [email protected] version (the earlier version that does not use fs.readFileSync(.../core.json)? If so, then I think we could close this issue for now.

I'll continue to follow up on browserify/resolve#297 and eventually update to require-in-the-middle to use [email protected] (once there is a non-prerelease release of it).

@polRk
Copy link
Author

polRk commented May 25, 2023

Can you confirm that things work for you now that the maintainer of the resolve module has moved the "latest"

In my environment "latest" and non exact tags a banned. It will be nice to fix resolve package version in require-in-the-middle.

@polRk
Copy link
Author

polRk commented May 25, 2023

[email protected] version (the earlier version that does not use fs.readFileSync(.../core.json)? If so, then I think we could close this issue for now.

thank you, works fine

@trentm
Copy link
Contributor

trentm commented May 26, 2023

Thanks for getting back. I'll continue to follow up on the require-in-the-middle issue. @dyladan I think you can close this issue now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage
Projects
None yet
Development

No branches or pull requests

4 participants