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

BUG - Request telemetry is not collected #1375

Closed
zhiyuanliang-ms opened this issue Aug 30, 2024 · 6 comments
Closed

BUG - Request telemetry is not collected #1375

zhiyuanliang-ms opened this issue Aug 30, 2024 · 6 comments

Comments

@zhiyuanliang-ms
Copy link
Contributor

zhiyuanliang-ms commented Aug 30, 2024

  • Package Name: applicationinsights
  • Package Version: 3.2.2
  • Node.js Version: 20
  • Bug description: No request telemetry when using applicationinsights in ESM code

The following cjs code works as expected. I can see live metrics and a custom event in the app insights. More importantly, I can see request telemetry if I sent get request to localhost:5000 because AutoCollectRequest config is enabled by default. (ref).

const appInsights = require("applicationinsights");

appInsights.setup(connectionString)
    .setSendLiveMetrics(true)
    .start();

appInsights.defaultClient.trackEvent({name: "test"})

const express = require('express');
const app = express();
const port = 5000;


app.get('/', (req, res) => {
  const targetingId = req.headers['targetingid'];
  res.send(`id: ${targetingId}`);
});

app.listen(port, () => {
  console.log(`Server is running on http://localhost:${port}`);
});

However, if I rewrite the above code in ESM style as below:

import applicationInsights from "applicationinsights"; // default import (module.default)
applicationInsights.setup(connectionString)
  .setSendLiveMetrics(true)
  .start();

applicationInsights.defaultClient.trackEvent({name: "test"})

import express from 'express';

const app = express();
const port = 5000;

app.get('/', (req, res) => {
  const targetingId = req.headers['targetingid'];
  res.send(`id: ${targetingId}`);
});

app.listen(port, () => {
  console.log(`Server is running on http://localhost:${port}`);
});

I still can see some of the live metrics (e.g. CPU usage) and the custom event (except for the warning log mentioned in #1324).

BUT, I CANNOT see any request telemetry in application insights. After debugging for a while, I found that applicationinsights will create a default TelemetryClient and while initializing it, applicationinsights will call UseAzureMonitor method provided by @azure/monitor-opentelemetry and it seems like azure monitor is supposed to automatically collect all http request. But it turns out to do nothing when applicationinsights is imported in ESM.

I also tested applicationinsights v2 (classic version). Everything works as expected in ESM when I used applicationinsights 2.9.x. The applicationinsights v3 is built on azure opentelemetry monitor. I guess it is the root cause of the bug.

I also noticed that #1354 called out that applicationinsights package is incompatible with ESM module system. ESM is the trending of JavaScript world. IMO, ESM scenario is more important than CJS scenario. Could you help investigate this bug? Any clue or information will be appreciated.

@JacksonWeber
Copy link
Contributor

@zhiyuanliang-ms This is actually related to a larger issue in OpenTelemetry-JS of loading the @opentelemetry/instrumentation package before the instrumentation libraries that need to be instrumented. The current solution for support auto-instrumentation with OpenTelemetry also fixes the issue with this package. The current solution is documented here in the OpenTelemetry project.

Please let me know if setting the NODE_OPTIONS with the appropriate loader hook works for you.

@zhiyuanliang-ms
Copy link
Contributor Author

Thank you, @JacksonWeber It works.

I created a seperated file telemetry.mjs as below

import appInsights from "applicationinsights"; // default import (module.default)
appInsights.setup(connectionString)
  .setAutoCollectRequests(true) // not work for esm
  .setSendLiveMetrics(true)
  .start();

appInsights.defaultClient.trackEvent({name: "test"})

I have another server.mjs:

import express from 'express';

const app = express();
const port = 5000;

app.get('/', (req, res) => {
  const targetingId = req.headers['targetingid'];
  res.send(`id: ${targetingId}`);
});

app.listen(port, () => {
  console.log(`Server is running on http://localhost:${port}`);
});

I used node --experimental-loader=@opentelemetry/instrumentation/hook.mjs --import ./telemetry.mjs ./server.mjs
Then, I can see the incoming request in the app insights.

@JacksonWeber
Copy link
Contributor

Closing based on the above solution working.

@olli-quartal
Copy link

Hi @JacksonWeber,

Would it be possible to get an alert about this to the Readme page? I just spent at least 8 hours trying to figure out why on earth Express application is not logging.

Though the examples were require, this is quite common with all libraries and they typically work also with imports. I could not have imagined that a library that is actively maintained by Microsoft would not support ESM in 2024. So I was trying a huge amount of different things before realizing that it works with require, but not with ESM import which we use in all our development. Especially now that the issue is closed, this is really difficult to find.

We will probably use 2.x until this works with ESM.

@JacksonWeber
Copy link
Contributor

@olli-quartal as mentioned in the issue linked above, this is related to OpenTelemetry (which version 3.X SDK is built on). Does the solution mentioned there not work for you or fit your use-case?

@olli-quartal
Copy link

Hi @JacksonWeber,

The solution might probably work, or then we just use an older version to avoid hacking with Node experimental features....

However, my point is that for someone coming from outside - just expecting to use a Microsoft product with Microsoft maintained library - such a behavior is extremely unexpected. Normally, you would expect ESM modules to just work. And you would expect such a simple thing as adding logging to a web site just work by following instructions. Now it just fails and you need to spend hours searching what is wrong (first you look at the configurations, then permissions etc.). Finally finding an answer from a bug report that is already closed and there is the link to open telemetry web site.

So to avoid this, there should be a clear and visible warning on the documentation page that this is now the current beavior / limitation for ESM. And here is the link how to hack around it using experimental Node features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants