Skip to content
This repository has been archived by the owner on Mar 7, 2018. It is now read-only.

Integrate with application insights #56

Merged
merged 7 commits into from Jul 6, 2017
Merged

Integrate with application insights #56

merged 7 commits into from Jul 6, 2017

Conversation

c-w
Copy link
Contributor

@c-w c-w commented Jul 5, 2017

We make use of three application insights apis:

  • trackTrace for log statements
  • trackDependency for call to third-party services
  • trackEvent for first-party scenarios

Copy link
Contributor

@Smarker Smarker left a comment

Choose a reason for hiding this comment

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

LGTM with comments

.then(returnValue => {
const duration = new Date() - start;
const success = true;
client.trackEvent(eventName, { duration: duration, success: success });
Copy link
Contributor

Choose a reason for hiding this comment

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

What if you wanted to add other properties to the event like siteType? Or metrics like the number of inserts performed? Would it be more flexible to add a third optional parameter to trackEvent for properties and a fourth optional parameter to it for metrics?

Copy link
Contributor Author

@c-w c-w Jul 6, 2017

Choose a reason for hiding this comment

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

Done in 8f9e920 79fd918.

appInsights.setup(appInsightsKey);
appInsights.start();
client = appInsights.getClient(appInsightsKey);
console.log = trackTrace(/* INFO */ 1, consoleLog);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would enumerating the values 1, 2, 3 as INFO, ERROR, WARNING be more clear?

Copy link
Contributor Author

@c-w c-w Jul 6, 2017

Choose a reason for hiding this comment

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

@c-w c-w merged commit 754729c into master Jul 6, 2017
@c-w c-w deleted the application-insights branch July 6, 2017 21:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants