From ccb0841a50862b7cb76de48fe2f7d02001976a7f Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Fri, 17 Mar 2023 11:38:42 +0000 Subject: [PATCH] Merge pull request #12281 from overleaf/jpa-tweak-event-segmentation-filter [web] tweak analytics event segmentation filter GitOrigin-RevId: e00fef0ac74edfd7fbace33bf9289f1c6f905b57 --- .../Features/Analytics/AnalyticsManager.js | 34 +++++++++++++++---- .../test/acceptance/src/helpers/InitApp.js | 4 +-- .../src/Analytics/AnalyticsManagerTests.js | 26 ++++++++++++++ 3 files changed, 55 insertions(+), 9 deletions(-) diff --git a/services/web/app/src/Features/Analytics/AnalyticsManager.js b/services/web/app/src/Features/Analytics/AnalyticsManager.js index a09b40fe25..d5c9cbfbc0 100644 --- a/services/web/app/src/Features/Analytics/AnalyticsManager.js +++ b/services/web/app/src/Features/Analytics/AnalyticsManager.js @@ -6,7 +6,7 @@ const Queues = require('../../infrastructure/Queues') const crypto = require('crypto') const _ = require('lodash') const { expressify } = require('../../util/promises') -const { logger } = require('@overleaf/logger') +const logger = require('@overleaf/logger') const { getAnalyticsIdFromMongoUser } = require('./AnalyticsHelper') const analyticsEventsQueue = Queues.getQueue('analytics-events') @@ -132,6 +132,10 @@ function updateEditingSession(userId, projectId, countryCode, segmentation) { return } if (!_isSegmentationValid(segmentation)) { + logger.info( + { userId, projectId, segmentation }, + 'rejecting analytics editing session due to bad segmentation' + ) return } Metrics.analyticsQueue.inc({ @@ -165,9 +169,17 @@ function _recordEvent( { delay } = {} ) { if (!_isAttributeValid(event)) { + logger.info( + { analyticsId, event, segmentation }, + 'rejecting analytics event due to bad event name' + ) return } if (!_isSegmentationValid(segmentation)) { + logger.info( + { analyticsId, event, segmentation }, + 'rejecting analytics event due to bad segmentation' + ) return } Metrics.analyticsQueue.inc({ status: 'adding', event_type: 'event' }) @@ -193,10 +205,18 @@ function _recordEvent( } function _setUserProperty({ analyticsId, propertyName, propertyValue }) { - if ( - !_isAttributeValid(propertyName) || - !_isAttributeValueValid(propertyValue) - ) { + if (!_isAttributeValid(propertyName)) { + logger.info( + { analyticsId, propertyName, propertyValue }, + 'rejecting analytics user property due to bad name' + ) + return + } + if (!_isAttributeValueValid(propertyValue)) { + logger.info( + { analyticsId, propertyName, propertyValue }, + 'rejecting analytics user property due to bad value' + ) return } Metrics.analyticsQueue.inc({ @@ -254,8 +274,8 @@ function _isAttributeValueValid(attributeValue) { } function _isSegmentationValueValid(attributeValue) { - // spaces are allowed for segmentation values - return !attributeValue || /^[a-zA-Z0-9-_.:;,/ ]+$/.test(attributeValue) + // spaces and %-escaped values are allowed for segmentation values + return !attributeValue || /^[a-zA-Z0-9-_.:;,/ %]+$/.test(attributeValue) } function _isSegmentationValid(segmentation) { diff --git a/services/web/test/acceptance/src/helpers/InitApp.js b/services/web/test/acceptance/src/helpers/InitApp.js index e04db24545..d5e0bcf218 100644 --- a/services/web/test/acceptance/src/helpers/InitApp.js +++ b/services/web/test/acceptance/src/helpers/InitApp.js @@ -2,14 +2,14 @@ const App = require('../../../../app.js') const QueueWorkers = require('../../../../app/src/infrastructure/QueueWorkers') const MongoHelper = require('./MongoHelper') const RedisHelper = require('./RedisHelper') -const { logger } = require('@overleaf/logger') +const logger = require('@overleaf/logger') const Settings = require('@overleaf/settings') const MockReCAPTCHAApi = require('../mocks/MockReCaptchaApi') const { gracefulShutdown, } = require('../../../../app/src/infrastructure/GracefulShutdown') -logger.level('error') +logger.logger.level('error') MongoHelper.initialize() RedisHelper.initialize() diff --git a/services/web/test/unit/src/Analytics/AnalyticsManagerTests.js b/services/web/test/unit/src/Analytics/AnalyticsManagerTests.js index f15806e50a..a907477ae8 100644 --- a/services/web/test/unit/src/Analytics/AnalyticsManagerTests.js +++ b/services/web/test/unit/src/Analytics/AnalyticsManagerTests.js @@ -114,6 +114,7 @@ describe('AnalyticsManager', function () { 'fr', { key: '' } ) + sinon.assert.called(this.logger.info) sinon.assert.notCalled(this.analyticsEditingSessionQueue.add) }) @@ -122,6 +123,7 @@ describe('AnalyticsManager', function () { this.fakeUserId, 'not an event!' ) + sinon.assert.called(this.logger.info) sinon.assert.notCalled(this.analyticsEventsQueue.add) }) @@ -131,6 +133,7 @@ describe('AnalyticsManager', function () { 'an_event', { not_a: 'Valid Segmentation!' } ) + sinon.assert.called(this.logger.info) sinon.assert.notCalled(this.analyticsEventsQueue.add) }) @@ -140,6 +143,7 @@ describe('AnalyticsManager', function () { 'an invalid property', 'a_value' ) + sinon.assert.called(this.logger.info) sinon.assert.notCalled(this.analyticsUserPropertiesQueue.add) }) @@ -149,6 +153,7 @@ describe('AnalyticsManager', function () { 'a_property', 'an invalid value' ) + sinon.assert.called(this.logger.info) sinon.assert.notCalled(this.analyticsUserPropertiesQueue.add) }) }) @@ -157,6 +162,7 @@ describe('AnalyticsManager', function () { it('identifyUser', function () { const analyticsId = 'bd101c4c-722f-4204-9e2d-8303e5d9c120' this.AnalyticsManager.identifyUser(this.fakeUserId, analyticsId, true) + sinon.assert.notCalled(this.logger.info) sinon.assert.calledWithMatch( this.Queues.createScheduledJob, 'analytics-events', @@ -180,6 +186,7 @@ describe('AnalyticsManager', function () { event, null ) + sinon.assert.notCalled(this.logger.info) sinon.assert.calledWithMatch(this.analyticsEventsQueue.add, 'event', { analyticsId: this.analyticsId, event, @@ -198,6 +205,7 @@ describe('AnalyticsManager', function () { countryCode, segmentation ) + sinon.assert.notCalled(this.logger.info) sinon.assert.calledWithMatch( this.analyticsEditingSessionQueue.add, 'editing-session', @@ -217,6 +225,7 @@ describe('AnalyticsManager', function () { 'an_event', { compileTime: timings?.compileE2E } ) + sinon.assert.notCalled(this.logger.info) sinon.assert.calledWithMatch(this.analyticsEventsQueue.add, 'event', { analyticsId: this.analyticsId, event: 'an_event', @@ -231,6 +240,7 @@ describe('AnalyticsManager', function () { 'an_event', { segment: 'a value with spaces' } ) + sinon.assert.notCalled(this.logger.info) sinon.assert.calledWithMatch(this.analyticsEventsQueue.add, 'event', { analyticsId: this.analyticsId, event: 'an_event', @@ -239,12 +249,28 @@ describe('AnalyticsManager', function () { }) }) + it('percent sign in event segmentation value', async function () { + await this.AnalyticsManager.recordEventForUser( + this.fakeUserId, + 'an_event', + { segment: 'a value with escaped comma %2C' } + ) + sinon.assert.notCalled(this.logger.info) + sinon.assert.calledWithMatch(this.analyticsEventsQueue.add, 'event', { + analyticsId: this.analyticsId, + event: 'an_event', + segmentation: { segment: 'a value with escaped comma %2C' }, + isLoggedIn: true, + }) + }) + it('boolean field in event segmentation', async function () { await this.AnalyticsManager.recordEventForUser( this.fakeUserId, 'an_event', { isAutoCompile: false } ) + sinon.assert.notCalled(this.logger.info) sinon.assert.calledWithMatch(this.analyticsEventsQueue.add, 'event', { analyticsId: this.analyticsId, event: 'an_event',