From 184be9387fd2153299df0f30e67a1d94338b99dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Wed, 26 Jun 2024 15:49:13 +0200 Subject: [PATCH] (core) Enabling telemetry on /api/version endpoint 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 --- app/server/lib/FlexServer.ts | 2 +- app/server/lib/UpdateManager.ts | 5 +--- app/server/mergedServerMain.ts | 2 +- test/client/lib/UrlState.ts | 5 ---- test/client/models/gristUrlState.ts | 2 -- test/gen-server/UpdateChecks.ts | 40 ++++++++++++++++++++++++++--- test/server/Comm.ts | 10 -------- test/server/lib/DocApi.ts | 2 +- 8 files changed, 41 insertions(+), 27 deletions(-) diff --git a/app/server/lib/FlexServer.ts b/app/server/lib/FlexServer.ts index 46e4a508..015b3c11 100644 --- a/app/server/lib/FlexServer.ts +++ b/app/server/lib/FlexServer.ts @@ -1971,7 +1971,7 @@ export class FlexServer implements GristServer { } public addUpdatesCheck() { - if (this._check('update')) { return; } + if (this._check('update', 'json')) { return; } // For now we only are active for sass deployments. if (this._deploymentType !== 'saas') { return; } diff --git a/app/server/lib/UpdateManager.ts b/app/server/lib/UpdateManager.ts index c7ac9f67..a79c5b2c 100644 --- a/app/server/lib/UpdateManager.ts +++ b/app/server/lib/UpdateManager.ts @@ -85,15 +85,12 @@ export class UpdateManager { 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 - // with the version of the client. Won't be send without telemetry opt in. + // with the version of the client. const deploymentId = optStringParam( payload("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). const deploymentType = optStringParam( payload("deploymentType"), diff --git a/app/server/mergedServerMain.ts b/app/server/mergedServerMain.ts index 8405cd8d..987343f6 100644 --- a/app/server/mergedServerMain.ts +++ b/app/server/mergedServerMain.ts @@ -106,7 +106,6 @@ export async function main(port: number, serverTypes: ServerType[], server.addHealthCheck(); if (includeHome || includeApp) { server.addBootPage(); - server.addUpdatesCheck(); } server.denyRequestsIfNotReady(); @@ -148,6 +147,7 @@ export async function main(port: number, serverTypes: ServerType[], server.addDocApiForwarder(); } server.addJsonSupport(); + server.addUpdatesCheck(); await server.addLandingPages(); // todo: add support for home api to standalone app server.addHomeApi(); diff --git a/test/client/lib/UrlState.ts b/test/client/lib/UrlState.ts index 5de50c2e..356c3c0e 100644 --- a/test/client/lib/UrlState.ts +++ b/test/client/lib/UrlState.ts @@ -1,14 +1,11 @@ -import * as log from 'app/client/lib/log'; import {HistWindow, UrlState} from 'app/client/lib/UrlState'; import {assert} from 'chai'; import {dom} from 'grainjs'; import {popGlobals, pushGlobals} from 'grainjs/dist/cjs/lib/browserGlobals'; import {JSDOM} from 'jsdom'; import fromPairs = require('lodash/fromPairs'); -import * as sinon from 'sinon'; describe('UrlState', function() { - const sandbox = sinon.createSandbox(); let mockWindow: HistWindow; 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. const jsdomDoc = new JSDOM(""); pushGlobals(jsdomDoc.window); - sandbox.stub(log, 'debug'); }); afterEach(function() { popGlobals(); - sandbox.restore(); }); interface State { diff --git a/test/client/models/gristUrlState.ts b/test/client/models/gristUrlState.ts index c7065fef..764b00fd 100644 --- a/test/client/models/gristUrlState.ts +++ b/test/client/models/gristUrlState.ts @@ -1,4 +1,3 @@ -import * as log from 'app/client/lib/log'; import {HistWindow, UrlState} from 'app/client/lib/UrlState'; import {getLoginUrl, UrlStateImpl} from 'app/client/models/gristUrlState'; import {IGristUrlState} from 'app/common/gristUrls'; @@ -42,7 +41,6 @@ describe('gristUrlState', function() { // These grainjs browserGlobals are needed for using dom() in tests. const jsdomDoc = new JSDOM(""); pushGlobals(jsdomDoc.window); - sandbox.stub(log, 'debug'); }); afterEach(function() { diff --git a/test/gen-server/UpdateChecks.ts b/test/gen-server/UpdateChecks.ts index cdd9426f..53441f8c 100644 --- a/test/gen-server/UpdateChecks.ts +++ b/test/gen-server/UpdateChecks.ts @@ -5,10 +5,12 @@ import * as sinon from 'sinon'; import { configForUser } from "test/gen-server/testUtils"; import * as testUtils from "test/server/testUtils"; import { Defer, serveSomething, Serving } from "test/server/customUtil"; +import { Telemetry } from 'app/server/lib/Telemetry'; import { Deps } from "app/server/lib/UpdateManager"; import { TestServer } from "test/gen-server/apiUtils"; import { delay } from "app/common/delay"; import { LatestVersion } from 'app/common/InstallAPI'; +import { TelemetryEvent, TelemetryMetadataByLevel } from 'app/common/Telemetry'; const assert = chai.assert; @@ -21,8 +23,13 @@ const stop = async () => { let homeUrl: string; let dockerHub: Serving & { signal: () => Defer }; +let sandbox: sinon.SinonSandbox; +const logMessages: [TelemetryEvent, TelemetryMetadataByLevel?][] = []; const chimpy = configForUser("Chimpy"); +const headers = { + headers: {'Content-Type': 'application/json'} +}; // Tests specific complex scenarios that may have previously resulted in wrong behavior. describe("UpdateChecks", function () { @@ -30,8 +37,6 @@ describe("UpdateChecks", function () { this.timeout("20s"); - const sandbox = sinon.createSandbox(); - before(async function () { testUtils.EnvironmentSnapshot.push(); dockerHub = await dummyDockerHub(); @@ -41,11 +46,19 @@ describe("UpdateChecks", function () { Object.assign(process.env, { GRIST_TEST_SERVER_DEPLOYMENT_TYPE: "saas", }); + sandbox = sinon.createSandbox(); sandbox.stub(Deps, "REQUEST_TIMEOUT").value(300); sandbox.stub(Deps, "RETRY_TIMEOUT").value(400); sandbox.stub(Deps, "GOOD_RESULT_TTL").value(500); sandbox.stub(Deps, "BAD_RESULT_TTL").value(200); 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); }); @@ -69,7 +82,7 @@ describe("UpdateChecks", function () { assert.equal(result.latestVersion, "10"); // 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.deepEqual(resp2.data, result); }); @@ -197,6 +210,27 @@ describe("UpdateChecks", function () { assert.equal(resp.status, 500); 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() { diff --git a/test/server/Comm.ts b/test/server/Comm.ts index 51e21a0e..eb979486 100644 --- a/test/server/Comm.ts +++ b/test/server/Comm.ts @@ -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() { // Run the cleanup callbacks registered in cleanup(). await Promise.all(cleanup.splice(0).map(callback => callback())); diff --git a/test/server/lib/DocApi.ts b/test/server/lib/DocApi.ts index b816dc3f..86b515d3 100644 --- a/test/server/lib/DocApi.ts +++ b/test/server/lib/DocApi.ts @@ -4029,7 +4029,7 @@ function testDocApi() { ...pick(options, 'name', 'memo', 'enabled', 'watchedColIds'), }, chimpy ); - assert.equal(status, 200); + assert.equal(status, 200, `Error during subscription: ` + JSON.stringify(data)); return data as WebhookSubscription; }