Adding Application Insights support to base images #18

lostintangent · 2017-06-27T07:10:29Z

This PR updates the base Node.js images, by adding the Application Insights SDK to them. If the Application Insights integration is enabled for the "hosting" web app (resulting in the ENABLE_APPINSIGHTS env var being set), then the app's startup command will be augmented in order to transparently initialize the Application Insights runtime in-proc. This way, users can enable AI when creating a web app, do a Git push to a default runtime image, and start immediately collecting telemetry, without needing to do anything further. 🚀

If the AI integration feature isn't enabled, then the app will run as usual, with the SDK bits remaining completely inert within the Docker container (similar to the existing default web site bits).

Note: This PR currently only updates the Node 8.1.2 image, in order to get feedback on the implementation. Once the general approach is validated/refined, I'll update the PR to include support for the rest of the default images.

// CC @naziml @nickwalkmsft

lostintangent · 2017-06-27T07:13:39Z

8.1/startup/generateStartupCommand.js

@@ -18,15 +18,45 @@ if (typeof process.env.WEBSITE_ROLE_INSTANCE_ID !== 'undefined'
    roleInstanceId = process.env.WEBSITE_ROLE_INSTANCE_ID;
 }

-var startupCommand = fs.readFileSync(CMDFILE, 'utf8').trim();
+// Is Application Insights enabled, with an associated ikey?
+var appInsightsEnabled = process.env.ENABLE_APPINSIGHTS && process.env.APPINSIGHTS_INSTRUMENTATIONKEY;


Per our previous discussion, the expectation is that if the ENABLE_APPINSIGHTS is set, then an Application Insights resource has been provisioned, and its associated instrumentation key has been specified via the APPINSIGHTS_INSTRUMENTATIONKEY variable (which the Application Insights SDK auto-detects).

lostintangent · 2017-06-27T07:15:28Z

8.1/startup/generateStartupCommand.js

+
+    // The command is using an unknown executable, and therefore,
+    // the App Insights runtime can't be automatically enabled.
+    return command;


This is primarily meant to support the NPM start script scenario, where the app could specific an arbitrary command, that I'm not aware of a deterministic way to bootstrap Application Insights into. In practice, accommodating PM2 and Node should cover most scenarios.

This should be fine for now.

lostintangent · 2017-06-27T07:17:08Z

8.1/startup/initAppInsights.js

+    // Initialize the Application Insights runtime, using the
+    // instrumentation key provided by the environment.
+    require("applicationinsights").setup().start();
+} catch (error) {


Since I check for the presence of the ikey before loading this script, there aren't are known reasons why this would fail. However, I added this error swallowing logic, since I assume we want to be sure this feature add-on doesn't impact the stability of the app itself.

lostintangent · 2017-06-27T07:22:37Z

8.1/startup/package.json

@@ -4,6 +4,7 @@
  "description": "Express-driven static site hosted from default App Service wwwroot directory",
  "main": "default-static-site.js",
  "dependencies": {
+    "applicationinsights": "0.21.0",


I decided to add the AI SDK dependency here, since the Dockerfile was already running npm install in this directory, and it actually seemed logical to couple any "in-box" dependencies with the startup directory.

Additionally, while I used a static version for this dependency, I'm cool with making it pick up patch changes if we thought that made sense. Since these images would need to be rebuilt to pull in any updated dependencies anyways, I figured it made sense to just be explicit about the version being bundled. Would love feedback though.

lostintangent · 2017-06-27T07:37:03Z

8.1/startup/generateStartupCommand.js

@@ -47,7 +77,7 @@ if (!startupCommand) {
 if (!startupCommand) {
    console.log("No startup command or autodetected startup script " +
        "found. Running default static site.");
-    startupCommand = "node " + DEFAULTAPP;
+    startupCommand = nodeCommandPrefix + DEFAULTAPP;


We could potentially skip initializing the AI runtime here, since it's just a "getting started" app. But, if the user enabled AI, they might expect to be able to see the HTTP traffic for the starter, in order to get a feel for the experience, before they push their own app code. Feedback would be appreciated here.

lostintangent · 2017-06-27T07:39:25Z

8.1/startup/generateStartupCommand.js

+    return command;
+}
+
+var startupCommand = augmentCommandIfNeccessary(fs.readFileSync(CMDFILE, 'utf8').trim());


Besides pm2 start <process.json> and node <startScript>, are there any other startup command formats that could be provided by the user and passed in when the Docker container is run? I'm not aware of any, but I just wanted to confirm.

nickwalkmsft

Cool stuff. This should be a good enhancement for basic scenarios.

nickwalkmsft · 2017-07-18T19:34:18Z

8.1.2/startup/generateStartupCommand.js

-        console.log("Found scripts.start in package.json")
-        startupCommand = 'npm start';
+        console.log("Found scripts.start in package.json");
+        startupCommand = augmentCommandIfNeccessary(json.scripts.start.trim());


I'm assuming there's no great difference between running npm start and extracting the command to run it directly?

The only difference that I can think of is that npm start adds ./node_modules/.bin to the process' PATH, and so it's possible that we could introduce issues for apps that depended on that behavior. However, that's typically only used to be able to run executables in the script that are actually installed as a local Node.js module (e.g. webpack ), so I don't think this should be a problem in practice.

The bigger issue might be the assumption that the start script was running node in the first place, since you could technically put anything else there (e.g. foobar -f server.js).

After thinking about this further, and looking at a set of example apps that have start scripts, I think this logic is sufficient.

Better late than never. Another init option for the key is APPLICATIONINSIGHTS_CONNECTION_STRING as seen from the library documentation.

nickwalkmsft · 2017-07-18T19:37:05Z

8.1.2/startup/initAppInsights.js

+try {
+    // Initialize the Application Insights runtime, using the
+    // instrumentation key provided by the environment.
+    require("applicationinsights").setup().start();


Does this cause any issues if the user is already doing it in their app?

Also - what does this accomplish for your app if you don't have it baked in? I'm assuming it tracks basic metrics?

Yep, doing this init step auto-registers a bunch of metric collection, in order to track telemetry without your app code needing to do anything at all.

If the user was already initializing the AI SDK, then the second call should effectively no-op, but let me verify that.

Can the SDK take any kind of configuration in these calls? If the user is using the SDK, I think their call to it would be the second call (assuming --require would run before anything in the app); if they have done any kind of configuration, that should take precedence.

Yep, the app's call to setup().start() would occur after the bootstrap's script, and therefore, any config that was specified wouldn't take effect unfortunately. That said, the user could provide config via env vars/app settings, which would be automatically picked up/respected by the bootstrap script.

In order to support the scenario where the user chose to auto-enable AI when creating the web app, but also manually added the AI SDK to their app, we may need to make some additional changes to the AI SDK in order to update the config each time the SDK/runtime in initialized.

@nickwalkmsft So I confirmed that all AI SDK config can be overridable in app code (e.g. enabling certain types of auto-collected telemetry), except for the actual instrumentation key. However, the app could work around this by simply setting the APPINSIGHTS_INSTRUMENTATIONKEY environment variable (which is actually the recommendation for production anyways), and then everything would work as expected, since the env var would naturally be used by the "first" initialization of the AI SDK.

nickwalkmsft · 2017-07-19T00:01:03Z

Also, I just had a thought - how does this work on an arbitrary customer app that doesn't use AI? Won't the --require fail since they haven't installed it?

lostintangent · 2017-07-19T04:31:25Z

@nickwalkmsft Since the Docker image itself includes the AI SDK, the --require will always work. The --require arg can reference an arbitrary JS file/module, and that module can in turn reference NPM dependencies which are relative to it, not the app itself. So when we pre-load initAppInsights.js, and that file calls require("applicationinsights"), that applicationinsights module will be resolved from the directory that initiAppInsights.js is within, not the user code directory.

lostintangent · 2017-07-29T05:38:27Z

@nickwalkmsft @naziml Let me know if there's anything else you think should be addressed with this PR.

As far as doing some E2E testing, do you think we should just leave this in one Node version and validate the experience a bit further, before replicating it to every Node version? It's already gated behind the ENABLE_APPINSIGHTS env var, but just let me know what you think we should update the PR with support for all Node versions.

lostintangent · 2017-08-17T03:38:56Z

@nickwalkmsft Should we close this PR for now and re-discuss it in the future?

nickwalkmsft · 2017-08-17T17:26:07Z

Yeah, please go ahead and close. Keep your commits around though; they may be useful later. Thank you!

lostintangent added 2 commits June 26, 2017 23:54

Adding App Insights support

453be65

Adding lockfile

80a31d8

lostintangent commented Jun 27, 2017

View reviewed changes

naziml approved these changes Jul 10, 2017

View reviewed changes

lostintangent added 3 commits July 14, 2017 17:04

Merging with upstream

e550ae0

Fixing formatting changes

24f2093

Fixing formatting issues

51c0567

nickwalkmsft approved these changes Jul 18, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Application Insights support to base images #18

Adding Application Insights support to base images #18

lostintangent commented Jun 27, 2017 •

edited

Loading

lostintangent Jun 27, 2017

lostintangent Jun 27, 2017

naziml Jul 10, 2017

lostintangent Jun 27, 2017

lostintangent Jun 27, 2017

lostintangent Jun 27, 2017

lostintangent Jun 27, 2017

lostintangent Jun 27, 2017

nickwalkmsft left a comment

nickwalkmsft Jul 18, 2017

lostintangent Jul 18, 2017

lostintangent Jul 29, 2017

gabriel-kohen-by Sep 12, 2020

nickwalkmsft Jul 18, 2017

nickwalkmsft Jul 18, 2017

lostintangent Jul 18, 2017

nickwalkmsft Jul 18, 2017

lostintangent Jul 18, 2017

lostintangent Jul 29, 2017

nickwalkmsft commented Jul 19, 2017

lostintangent commented Jul 19, 2017

lostintangent commented Jul 29, 2017 •

edited

Loading

lostintangent commented Aug 17, 2017

nickwalkmsft commented Aug 17, 2017

Adding Application Insights support to base images #18

Are you sure you want to change the base?

Adding Application Insights support to base images #18

Conversation

lostintangent commented Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickwalkmsft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickwalkmsft commented Jul 19, 2017

lostintangent commented Jul 19, 2017

lostintangent commented Jul 29, 2017 • edited Loading

lostintangent commented Aug 17, 2017

nickwalkmsft commented Aug 17, 2017

lostintangent commented Jun 27, 2017 •

edited

Loading

lostintangent commented Jul 29, 2017 •

edited

Loading