Skip to content

Adding Application Insights support to base images #18

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.

Sign up for GitHub

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

Open
lostintangent wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Adding Application Insights support to base images #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
453be65
Adding App Insights support
lostintangent Jun 27, 2017
80a31d8
Adding lockfile
lostintangent Jun 27, 2017
e550ae0
Merging with upstream
lostintangent Jul 15, 2017
24f2093
Fixing formatting changes
lostintangent Jul 15, 2017
51c0567
Fixing formatting issues
lostintangent Jul 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 35 additions & 5 deletions 8.1.2/startup/generateStartupCommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
var appInsightsPreloadArg = "--require /opt/startup/initAppInsights.js";
var nodeCommandPrefix = "node ";

if (appInsightsEnabled) {
console.log("Application Insights enabled");
nodeCommandPrefix += appInsightsPreloadArg + " ";
}

function augmentCommandIfNeccessary(command) {
if (!command || !appInsightsEnabled) {
return command;
}

// Application Insights is enabled, so we need to
// to update the specified startup command to pre-load it.
if (command.indexOf("pm2 start ") === 0) {
return command += " --node-args='" + appInsightsPreloadArg + "'";
} else if (command.indexOf("node ") === 0) {
// Simply replacing the prefix allows the user to specify
// additional Node flags, in addition to the AI preload one.
return command.replace("node ", nodeCommandPrefix);
}

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

var startupCommand = augmentCommandIfNeccessary(fs.readFileSync(CMDFILE, 'utf8').trim());

// No user-provided startup command, check for scripts.start
if (!startupCommand) {
var packageJsonPath = "/home/site/wwwroot/package.json";
var json = fs.existsSync(packageJsonPath) && JSON.parse(fs.readFileSync(packageJsonPath, 'utf8'))
if (typeof json == 'object' && typeof json.scripts == 'object' && typeof json.scripts.start == 'string') {
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());
Copy link
Contributor

@nickwalkmsft nickwalkmsft Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@lostintangent lostintangent Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@lostintangent lostintangent Jul 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@gabriel-kohen-by gabriel-kohen-by Sep 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

Expand All @@ -37,7 +67,7 @@ if (!startupCommand) {
var filename = "/home/site/wwwroot/" + autos[i];
if (fs.existsSync(filename)) {
console.log("No startup command entered, but found " + filename);
startupCommand = "node " + filename;
startupCommand = nodeCommandPrefix + filename;
break;
}
}
Expand All @@ -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;
}

// If HTTP logging is enabled and it doesn't appear that the user has tried to do any
Expand Down
8 changes: 8 additions & 0 deletions 8.1.2/startup/initAppInsights.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
try {
// Initialize the Application Insights runtime, using the
// instrumentation key provided by the environment.
require("applicationinsights").setup().start();
Copy link
Contributor

@nickwalkmsft nickwalkmsft Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@nickwalkmsft nickwalkmsft Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@lostintangent lostintangent Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@nickwalkmsft nickwalkmsft Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@lostintangent lostintangent Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@lostintangent lostintangent Jul 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

} catch (error) {
// Swallow any errors, in order to safeguard the
// user's app from any unexpected exceptions.
}
Loading