Archives/gristlabs_grist-core
1
0
Fork 0
mirror of https://github.com/gristlabs/grist-core.git synced 2024-10-27 20:44:07 +00:00
Code Issues Projects Releases Wiki Activity

(core) Enabling telemetry on /api/version endpoint

Browse Source 
Summary:
Version API endpoint wasn't logging telemetry from POST requests. The issue was in registration
order, this endpoint was registered before `expressJson` and it couldn't read json body in the handler.

Test Plan: Added new test

Reviewers: paulfitz

Reviewed By: paulfitz

Subscribers: paulfitz

Differential Revision: https://phab.getgrist.com/D4277
This commit is contained in:
Jarosław Sadziński 2024-06-26 15:49:13 +02:00
parent 05214d8f9a
commit 184be9387f
8 changed files with 41 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/server/lib/FlexServer.ts
View File

@ -1971,7 +1971,7 @@ export class FlexServer implements GristServer {
} }
public addUpdatesCheck() { public addUpdatesCheck() {
if (this._check('update')) { return; } if (this._check('update', 'json')) { return; }
// For now we only are active for sass deployments. // For now we only are active for sass deployments.
if (this._deploymentType !== 'saas') { return; } if (this._deploymentType !== 'saas') { return; }

5
app/server/lib/UpdateManager.ts
View File

@ -85,15 +85,12 @@ export class UpdateManager {
const payload = (name: string) => req.body?.[name] ?? req.query[name]; const payload = (name: string) => req.body?.[name] ?? req.query[name];
// This is the most interesting part for us, to track installation ids and match them // This is the most interesting part for us, to track installation ids and match them
// with the version of the client. Won't be send without telemetry opt in. // with the version of the client.
const deploymentId = optStringParam( const deploymentId = optStringParam(
payload("installationId"), payload("installationId"),
"installationId" "installationId"
); );
// Current version of grist-core part of the client. Currently not used and not
// passed from the client.
// Deployment type of the client (we expect this to be 'core' for most of the cases). // Deployment type of the client (we expect this to be 'core' for most of the cases).
const deploymentType = optStringParam( const deploymentType = optStringParam(
payload("deploymentType"), payload("deploymentType"),

2
app/server/mergedServerMain.ts
View File

@ -106,7 +106,6 @@ export async function main(port: number, serverTypes: ServerType[],
server.addHealthCheck(); server.addHealthCheck();
if (includeHome || includeApp) { if (includeHome || includeApp) {
server.addBootPage(); server.addBootPage();
server.addUpdatesCheck();
} }
server.denyRequestsIfNotReady(); server.denyRequestsIfNotReady();
@ -148,6 +147,7 @@ export async function main(port: number, serverTypes: ServerType[],
server.addDocApiForwarder(); server.addDocApiForwarder();
} }
server.addJsonSupport(); server.addJsonSupport();
server.addUpdatesCheck();
await server.addLandingPages(); await server.addLandingPages();
// todo: add support for home api to standalone app // todo: add support for home api to standalone app
server.addHomeApi(); server.addHomeApi();

5
test/client/lib/UrlState.ts
View File

@ -1,14 +1,11 @@
import * as log from 'app/client/lib/log';
import {HistWindow, UrlState} from 'app/client/lib/UrlState'; import {HistWindow, UrlState} from 'app/client/lib/UrlState';
import {assert} from 'chai'; import {assert} from 'chai';
import {dom} from 'grainjs'; import {dom} from 'grainjs';
import {popGlobals, pushGlobals} from 'grainjs/dist/cjs/lib/browserGlobals'; import {popGlobals, pushGlobals} from 'grainjs/dist/cjs/lib/browserGlobals';
import {JSDOM} from 'jsdom'; import {JSDOM} from 'jsdom';
import fromPairs = require('lodash/fromPairs'); import fromPairs = require('lodash/fromPairs');
import * as sinon from 'sinon';
describe('UrlState', function() { describe('UrlState', function() {
const sandbox = sinon.createSandbox();
let mockWindow: HistWindow; let mockWindow: HistWindow;
function pushState(state: any, title: any, href: string) { function pushState(state: any, title: any, href: string) {
@ -26,12 +23,10 @@ describe('UrlState', function() {
// These grainjs browserGlobals are needed for using dom() in tests. // These grainjs browserGlobals are needed for using dom() in tests.
const jsdomDoc = new JSDOM("<!doctype html><html><body></body></html>"); const jsdomDoc = new JSDOM("<!doctype html><html><body></body></html>");
pushGlobals(jsdomDoc.window); pushGlobals(jsdomDoc.window);
sandbox.stub(log, 'debug');
}); });
afterEach(function() { afterEach(function() {
popGlobals(); popGlobals();
sandbox.restore();
}); });
interface State { interface State {

2
test/client/models/gristUrlState.ts
View File

@ -1,4 +1,3 @@
import * as log from 'app/client/lib/log';
import {HistWindow, UrlState} from 'app/client/lib/UrlState'; import {HistWindow, UrlState} from 'app/client/lib/UrlState';
import {getLoginUrl, UrlStateImpl} from 'app/client/models/gristUrlState'; import {getLoginUrl, UrlStateImpl} from 'app/client/models/gristUrlState';
import {IGristUrlState} from 'app/common/gristUrls'; import {IGristUrlState} from 'app/common/gristUrls';
@ -42,7 +41,6 @@ describe('gristUrlState', function() {
// These grainjs browserGlobals are needed for using dom() in tests. // These grainjs browserGlobals are needed for using dom() in tests.
const jsdomDoc = new JSDOM("<!doctype html><html><body></body></html>"); const jsdomDoc = new JSDOM("<!doctype html><html><body></body></html>");
pushGlobals(jsdomDoc.window); pushGlobals(jsdomDoc.window);
sandbox.stub(log, 'debug');
}); });
afterEach(function() { afterEach(function() {

40
test/gen-server/UpdateChecks.ts
View File

@ -5,10 +5,12 @@ import * as sinon from 'sinon';
import { configForUser } from "test/gen-server/testUtils"; import { configForUser } from "test/gen-server/testUtils";
import * as testUtils from "test/server/testUtils"; import * as testUtils from "test/server/testUtils";
import { Defer, serveSomething, Serving } from "test/server/customUtil"; import { Defer, serveSomething, Serving } from "test/server/customUtil";
import { Telemetry } from 'app/server/lib/Telemetry';
import { Deps } from "app/server/lib/UpdateManager"; import { Deps } from "app/server/lib/UpdateManager";
import { TestServer } from "test/gen-server/apiUtils"; import { TestServer } from "test/gen-server/apiUtils";
import { delay } from "app/common/delay"; import { delay } from "app/common/delay";
import { LatestVersion } from 'app/common/InstallAPI'; import { LatestVersion } from 'app/common/InstallAPI';
import { TelemetryEvent, TelemetryMetadataByLevel } from 'app/common/Telemetry';
const assert = chai.assert; const assert = chai.assert;
@ -21,8 +23,13 @@ const stop = async () => {
let homeUrl: string; let homeUrl: string;
let dockerHub: Serving & { signal: () => Defer }; let dockerHub: Serving & { signal: () => Defer };
let sandbox: sinon.SinonSandbox;
const logMessages: [TelemetryEvent, TelemetryMetadataByLevel?][] = [];
const chimpy = configForUser("Chimpy"); const chimpy = configForUser("Chimpy");
const headers = {
headers: {'Content-Type': 'application/json'}
};
// Tests specific complex scenarios that may have previously resulted in wrong behavior. // Tests specific complex scenarios that may have previously resulted in wrong behavior.
describe("UpdateChecks", function () { describe("UpdateChecks", function () {
@ -30,8 +37,6 @@ describe("UpdateChecks", function () {
this.timeout("20s"); this.timeout("20s");
const sandbox = sinon.createSandbox();
before(async function () { before(async function () {
testUtils.EnvironmentSnapshot.push(); testUtils.EnvironmentSnapshot.push();
dockerHub = await dummyDockerHub(); dockerHub = await dummyDockerHub();
@ -41,11 +46,19 @@ describe("UpdateChecks", function () {
Object.assign(process.env, { Object.assign(process.env, {
GRIST_TEST_SERVER_DEPLOYMENT_TYPE: "saas", GRIST_TEST_SERVER_DEPLOYMENT_TYPE: "saas",
}); });
sandbox = sinon.createSandbox();
sandbox.stub(Deps, "REQUEST_TIMEOUT").value(300); sandbox.stub(Deps, "REQUEST_TIMEOUT").value(300);
sandbox.stub(Deps, "RETRY_TIMEOUT").value(400); sandbox.stub(Deps, "RETRY_TIMEOUT").value(400);
sandbox.stub(Deps, "GOOD_RESULT_TTL").value(500); sandbox.stub(Deps, "GOOD_RESULT_TTL").value(500);
sandbox.stub(Deps, "BAD_RESULT_TTL").value(200); sandbox.stub(Deps, "BAD_RESULT_TTL").value(200);
sandbox.stub(Deps, "DOCKER_ENDPOINT").value(dockerHub.url + "/tags"); sandbox.stub(Deps, "DOCKER_ENDPOINT").value(dockerHub.url + "/tags");
sandbox.stub(Telemetry.prototype, 'logEvent').callsFake((_, name, meta) => {
if (name !== 'checkedUpdateAPI') {
return Promise.resolve();
}
logMessages.push([name, meta]);
return Promise.resolve();
});
await startInProcess(this); await startInProcess(this);
}); });
@ -69,7 +82,7 @@ describe("UpdateChecks", function () {
assert.equal(result.latestVersion, "10"); assert.equal(result.latestVersion, "10");
// Also works in post method. // Also works in post method.
const resp2 = await axios.post(`${homeUrl}/api/version`); const resp2 = await axios.post(`${homeUrl}/api/version`, {}, headers);
assert.equal(resp2.status, 200); assert.equal(resp2.status, 200);
assert.deepEqual(resp2.data, result); assert.deepEqual(resp2.data, result);
}); });
@ -197,6 +210,27 @@ describe("UpdateChecks", function () {
assert.equal(resp.status, 500); assert.equal(resp.status, 500);
assert.match(resp.data.error, /timeout/); assert.match(resp.data.error, /timeout/);
}); });
it("logs deploymentId and deploymentType", async function () {
logMessages.length = 0;
setEndpoint(dockerHub.url + "/tags");
const installationId = "randomInstallationId";
const deploymentType = "test";
const resp = await axios.post(`${homeUrl}/api/version`, {
installationId,
deploymentType
}, chimpy);
assert.equal(resp.status, 200);
assert.equal(logMessages.length, 1);
const [name, meta] = logMessages[0];
assert.equal(name, "checkedUpdateAPI");
assert.deepEqual(meta, {
full: {
deploymentId: installationId,
deploymentType,
},
});
});
}); });
async function dummyDockerHub() { async function dummyDockerHub() {

10
test/server/Comm.ts
View File

@ -102,16 +102,6 @@ describe('Comm', function() {
} }
}; };
beforeEach(function() {
// Silence console messages from client-side Comm.ts.
if (!process.env.VERBOSE) {
// TODO: This no longer works, now that 'log' is a more proper "module" object rather than
// an arbitrary JS object. Also used in a couple other tests where logs are no longer
// silenced.
sandbox.stub(log, 'debug');
}
});
afterEach(async function() { afterEach(async function() {
// Run the cleanup callbacks registered in cleanup(). // Run the cleanup callbacks registered in cleanup().
await Promise.all(cleanup.splice(0).map(callback => callback())); await Promise.all(cleanup.splice(0).map(callback => callback()));

2
test/server/lib/DocApi.ts
View File

@ -4029,7 +4029,7 @@ function testDocApi() {
...pick(options, 'name', 'memo', 'enabled', 'watchedColIds'), ...pick(options, 'name', 'memo', 'enabled', 'watchedColIds'),
}, chimpy }, chimpy
); );
assert.equal(status, 200); assert.equal(status, 200, `Error during subscription: ` + JSON.stringify(data));
return data as WebhookSubscription; return data as WebhookSubscription;
} }