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

New: Add endpoint/data contract to log telemetry to internal api #3830

Merged
merged 3 commits into from Jun 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/utils-telemetry/src/index.ts
@@ -1,3 +1,3 @@
export * from './activity';
export * from './app-insights';
export * from './telemetry-api';
export * from './hints';
Expand Up @@ -3,30 +3,23 @@ type Properties = { [key: string]: string };

type TelemetryItem = {
data: {
baseData: {
measurements: Measurements;
name: string;
properties: Properties;
ver?: number;
Copy link
Member

Choose a reason for hiding this comment

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

Is the version added automatically when sending the telemetry? I know we have a few metrics around this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this tracking webhint version? A bunch of other places (including webhint.io) had this hardcoded to 2, so I thought this was the v2 of the app insights REST API. If you look at Line 84 of this file (before change), you will see if set to 2.

Copy link
Member

Choose a reason for hiding this comment

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

TBH, I don't know. @antross it he one that took care of the telemetry and now you so probably you know best how it works 😅

Let's get #3837 merged first and then this one, sounds good?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good ... looks like that one is merged now, is this one good to go then?

};
baseType: string;
measurements: Measurements;
name: string;
properties: Properties;
};
iKey: string;
name: string;
type: string;
time: string;
};

const appInsightsEndpoint = 'https://dc.services.visualstudio.com/v2/track';
const telemetryEndPoint = 'https://webhint-telemetry.azurewebsites.net/api/log';
Copy link
Member

Choose a reason for hiding this comment

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

@hxlnt just want confirmation this is what we want: to use our Azure Function to report the telemetry to app insights so we can clean up any extra information that we do not want to store.
Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Also, we may eventually want to redirect a custom subdomain (like webhint.io/telemetry) to this endpoint to make it more official. I talked about that with Tony, but we decided that we can do that as part of another task.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM.


let nameKey = '';
let sendTimeout: NodeJS.Timeout | null = null;
let telemetryQueue: TelemetryItem[] = [];

let options = {
batchDelay: 15000,
defaultProperties: {} as Properties,
enabled: false,
instrumentationKey: '8ef2b55b-2ce9-4c33-a09a-2c3ef605c97d',
molant marked this conversation as resolved.
Show resolved Hide resolved
post: (url: string, data: string) => {
return Promise.resolve(200);
}
Expand All @@ -40,7 +33,6 @@ export const enabled = () => {
/** Initialize telemetry with the provided options */
export const initTelemetry = (opts: Partial<typeof options>) => {
options = { ...options, ...opts };
nameKey = options.instrumentationKey.replace(/-/g, '');
};

/** Enable or disable telemetry */
Expand All @@ -59,7 +51,7 @@ const sendTelemetry = async () => {
telemetryQueue = [];

try {
const status = await options.post(appInsightsEndpoint, data);
const status = await options.post(telemetryEndPoint, data);

/* istanbul ignore next */
if (status !== 200) {
Expand All @@ -70,24 +62,20 @@ const sendTelemetry = async () => {
}
};

const track = async (type: string, data: TelemetryItem['data']['baseData']) => {
const track = async (type: string, data: TelemetryItem['data']) => {
if (!enabled()) {
return;
}

telemetryQueue.push({
data: {
baseData: {
measurements: data.measurements,
name: data.name,
properties: { ...options.defaultProperties, ...data.properties },
ver: 2
},
baseType: `${type}Data`
measurements: data.measurements,
name: data.name,
properties: { ...options.defaultProperties, ...data.properties }
},
iKey: options.instrumentationKey,
name: `Microsoft.ApplicationInsights.${nameKey}.${type}`,
time: new Date().toISOString()
time: new Date().toISOString(),
type: `${type}Data`

});

if (!options.batchDelay) {
Expand Down
Expand Up @@ -3,15 +3,13 @@ import * as proxyquire from 'proxyquire';
import * as sinon from 'sinon';

test('It posts telemetry events in the correct format', async (t) => {
const instrumentationKey = 'foo';
const post = sinon.stub().resolves(200);

const { initTelemetry, trackEvent } = proxyquire('../src/app-insights', {}) as typeof import('../src/app-insights');
const { initTelemetry, trackEvent } = proxyquire('../src/telemetry-api', {}) as typeof import('../src/telemetry-api');

initTelemetry({
batchDelay: 0,
enabled: true,
instrumentationKey,
post
});

Expand All @@ -20,23 +18,20 @@ test('It posts telemetry events in the correct format', async (t) => {
const postData = JSON.parse(post.firstCall.args[1]);

t.is(post.callCount, 1);
t.is(post.firstCall.args[0], 'https://dc.services.visualstudio.com/v2/track');
t.is(post.firstCall.args[0], 'https://webhint-telemetry.azurewebsites.net/api/log');
t.is(postData.length, 1);
t.is(postData[0].iKey, instrumentationKey);
t.is(postData[0].name, `Microsoft.ApplicationInsights.${instrumentationKey.replace(/-/g, '')}.Event`);
t.is(postData[0].data.name, 'test-event');
t.is(postData[0].time, new Date(postData[0].time).toISOString());
});

test('It does not post telemetry events when disabled', async (t) => {
const instrumentationKey = 'foo';
const post = sinon.stub().resolves();

const { initTelemetry, trackEvent, updateTelemetry } = proxyquire('../src/app-insights', {}) as typeof import('../src/app-insights');
const { initTelemetry, trackEvent, updateTelemetry } = proxyquire('../src/telemetry-api', {}) as typeof import('../src/telemetry-api');

initTelemetry({
batchDelay: 0,
enabled: true,
instrumentationKey,
post
});

Expand All @@ -48,15 +43,13 @@ test('It does not post telemetry events when disabled', async (t) => {
});

test('It batches multiple telemetry events together', async (t) => {
const instrumentationKey = 'foo';
const post = sinon.stub().resolves(200);

const { initTelemetry, trackEvent } = proxyquire('../src/app-insights', {}) as typeof import('../src/app-insights');
const { initTelemetry, trackEvent } = proxyquire('../src/telemetry-api', {}) as typeof import('../src/telemetry-api');

initTelemetry({
batchDelay: 50,
enabled: true,
instrumentationKey,
post
});

Expand All @@ -72,6 +65,6 @@ test('It batches multiple telemetry events together', async (t) => {
const items = JSON.parse(post.firstCall.args[1]);

t.is(items.length, 2);
t.is(items[0].data.baseData.name, 'test-event-1');
t.is(items[1].data.baseData.name, 'test-event-2');
t.is(items[0].data.name, 'test-event-1');
t.is(items[1].data.name, 'test-event-2');
});