Skip to content

Commit f2509f4

Browse files
dschomdschom
committed
bug(admin-server): Broken logs
Because: - Some analytics were lost - A refactor broke logging This commit: - Restores mzlog.service.ts to its previous glory - Fixes wonky injection - Fixes circular ref in admin-panel gaurds that would sometimes cause esbuild error - Fixes project.json in libs/shared/sentry to avoid esbuild error
1 parent 1463f62 commit f2509f4

30 files changed

Lines changed: 202 additions & 154 deletions

‎libs/shared/mozlog/src/index.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,5 @@
22
* License, v. 2.0. If a copy of the MPL was not distributed with this
33
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
44

5-
export { MozLoggerService } from './lib/mozlog.service';
5+
export * from './lib/mozlog.service';
6+
export * from './lib/mozlog.module';

‎libs/shared/mozlog/src/lib/mozlog.module.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
/* This Source Code Form is subject to the terms of the Mozilla Public
2+
* License, v. 2.0. If a copy of the MPL was not distributed with this
3+
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
4+
5+
import { Module, Global } from '@nestjs/common';
6+
7+
import { MozLoggerService } from './mozlog.service';
8+
9+
@Global()
10+
@Module({
11+
providers: [MozLoggerService],
12+
exports: [MozLoggerService],
13+
})
14+
export class MozLoggerModule {}

‎libs/shared/mozlog/src/lib/mozlog.service.spec.ts‎

Lines changed: 39 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ describe('MozLoggerService', () => {
6060
expect(service).toBeDefined();
6161
expect(service).toBeInstanceOf(MozLoggerService);
6262
expect(mockMozLogDefault).toHaveBeenCalledWith(mockConfig);
63-
expect(mockMozLogLoggerFactory).toHaveBeenCalledWith('default');
63+
expect(mockMozLogLoggerFactory).toHaveBeenCalledWith('MozLoggerService');
6464
});
6565

6666
it('sets context', () => {
@@ -69,39 +69,61 @@ describe('MozLoggerService', () => {
6969
});
7070

7171
it('logs info', () => {
72-
service.info('info', {});
73-
expect(mockMozLog.info).toBeCalledWith('', { message: 'info', '0': {} });
72+
service.info('info');
73+
expect(mockMozLog.info).toBeCalledWith('info', {});
74+
});
75+
76+
it('logs info with data', () => {
77+
service.info('info', { foo: 'bar', baz: 1 });
78+
expect(mockMozLog.info).toBeCalledWith('info', { foo: 'bar', baz: 1 });
79+
});
80+
81+
it('logs info for atypical message', () => {
82+
service.info({ weird: 'message' }, { foo: 'bar', baz: 1 });
83+
expect(mockMozLog.info).toBeCalledWith('', {
84+
message: { weird: 'message' },
85+
'0': { foo: 'bar', baz: 1 },
86+
});
87+
});
88+
89+
it('logs info for atypical number of args', () => {
90+
service.info({ weird: 'message' }, { foo: 'bar' }, { baz: 1 });
91+
expect(mockMozLog.info).toBeCalledWith('', {
92+
message: { weird: 'message' },
93+
'0': { foo: 'bar' },
94+
'1': { baz: 1 },
95+
});
7496
});
7597

7698
it('logs debug', () => {
77-
service.debug('debug', {});
78-
expect(mockMozLog.debug).toBeCalledWith('', { message: 'debug', '0': {} });
99+
service.debug('debug', { foo: 'bar', baz: 1 });
100+
expect(mockMozLog.debug).toBeCalledWith('debug', { foo: 'bar', baz: 1 });
79101
});
80102
it('logs error', () => {
81-
service.error('error', {});
82-
expect(mockMozLog.error).toBeCalledWith('', { message: 'error', '0': {} });
103+
service.error('error', { foo: 'bar', baz: 1 });
104+
expect(mockMozLog.error).toBeCalledWith('error', { foo: 'bar', baz: 1 });
83105
});
84106

85107
it('logs warn', () => {
86-
service.warn('warn', {});
87-
expect(mockMozLog.warn).toBeCalledWith('', { message: 'warn', '0': {} });
108+
service.warn('warn', { foo: 'bar', baz: 1 });
109+
expect(mockMozLog.warn).toBeCalledWith('warn', { foo: 'bar', baz: 1 });
88110
});
89111

90112
it('logs verbose', () => {
91-
service.verbose('verbose', {});
92-
expect(mockMozLog.verbose).toBeCalledWith('', {
93-
message: 'verbose',
94-
'0': {},
113+
service.verbose('verbose', { foo: 'bar', baz: 1 });
114+
expect(mockMozLog.verbose).toBeCalledWith('verbose', {
115+
foo: 'bar',
116+
baz: 1,
95117
});
96118
});
97119

98120
it('logs trace', () => {
99-
service.trace('trace', {});
100-
expect(mockMozLog.trace).toBeCalledWith('', { message: 'trace', '0': {} });
121+
service.trace('trace', { foo: 'bar', baz: 1 });
122+
expect(mockMozLog.trace).toBeCalledWith('trace', { foo: 'bar', baz: 1 });
101123
});
102124

103125
it('logs warning', () => {
104-
service.warn('warning', {});
105-
expect(mockMozLog.warn).toBeCalledWith('', { message: 'warning', '0': {} });
126+
service.warn('warning', { foo: 'bar', baz: 1 });
127+
expect(mockMozLog.warn).toBeCalledWith('warning', { foo: 'bar', baz: 1 });
106128
});
107129
});

‎libs/shared/mozlog/src/lib/mozlog.service.ts‎

Lines changed: 66 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -12,53 +12,107 @@ let logFactory: LoggerFactory;
1212
@Injectable({ scope: Scope.TRANSIENT })
1313
export class MozLoggerService implements LoggerService {
1414
private mozlog: MozLogger;
15+
public name: string;
1516

1617
constructor(
1718
configService: ConfigService,
18-
@Inject(INQUIRER) private parentClass: object
19+
@Inject(INQUIRER) parentClass: object
1920
) {
2021
if (!logFactory) {
2122
logFactory = mozlog(configService.get('log'));
2223
}
23-
this.mozlog = logFactory('default');
24-
if (this.parentClass?.constructor?.name) {
25-
this.setContext(this.parentClass.constructor.name);
24+
25+
if (parentClass?.constructor?.name) {
26+
this.name = parentClass?.constructor?.name;
27+
} else {
28+
this.name = 'default';
2629
}
30+
31+
this.mozlog = logFactory(this.name);
2732
}
2833

2934
public setContext(name: string): void {
35+
this.name = name;
3036
this.mozlog = logFactory(name);
3137
}
3238

3339
log(message: any, ...optionalParams: any[]): void {
34-
this.mozlog.info('', { message, ...optionalParams });
40+
const { type, data } = supportLegacyFormat(message, optionalParams);
41+
this.mozlog.info(type, data);
3542
}
3643

3744
info(message: any, ...optionalParams: any[]): void {
38-
this.mozlog.info('', { message, ...optionalParams });
45+
const { type, data } = supportLegacyFormat(message, optionalParams);
46+
this.mozlog.info(type, data);
3947
}
4048

4149
error(message: any, ...optionalParams: any[]): void {
42-
this.mozlog.error('', { message, ...optionalParams });
50+
const { type, data } = supportLegacyFormat(message, optionalParams);
51+
this.mozlog.error(type, data);
4352
}
4453

4554
warn(message: any, ...optionalParams: any[]): void {
46-
this.mozlog.warn('', { message, ...optionalParams });
55+
const { type, data } = supportLegacyFormat(message, optionalParams);
56+
this.mozlog.warn(type, data);
4757
}
4858

4959
debug(message: any, ...optionalParams: any[]): void {
50-
this.mozlog.debug('', { message, ...optionalParams });
60+
const { type, data } = supportLegacyFormat(message, optionalParams);
61+
this.mozlog.debug(type, data);
5162
}
5263

5364
verbose(message: any, ...optionalParams: any[]): void {
54-
this.mozlog.verbose('', { message, ...optionalParams });
65+
const { type, data } = supportLegacyFormat(message, optionalParams);
66+
this.mozlog.verbose(type, data);
5567
}
5668

5769
trace(message: any, ...optionalParams: any[]): void {
58-
this.mozlog.trace('', { message, ...optionalParams });
70+
const { type, data } = supportLegacyFormat(message, optionalParams);
71+
this.mozlog.trace(type, data);
5972
}
6073

6174
warning(message: any, ...optionalParams: any[]): void {
62-
this.mozlog.warn('', { message, ...optionalParams });
75+
const { type, data } = supportLegacyFormat(message, optionalParams);
76+
this.mozlog.warn(type, data);
77+
}
78+
}
79+
80+
/**
81+
* Handles legacy logging format which was intended for mozlog. Moz log calls were typically
82+
* in the form:
83+
*
84+
* > log(type, data);
85+
* > log(type)
86+
*
87+
* Where type was a discrete log type used for look up, and data was an optional dictionary
88+
* of values, a string containing a message, or an Error.
89+
*
90+
* As long as the logger is invoked in this way, this function ensures the resulting output
91+
* is compliant with what mozlog would have produced.
92+
*
93+
* @param message The message passed to the logger
94+
* @param optionalParams The remaining parameters passed to the logger
95+
* @returns { type:string, data:any } A mozlog compliant message
96+
*/
97+
export function supportLegacyFormat(message: any, optionalParams: any[]) {
98+
// This is the way most mozlog calls work, e.g log('some-type', {msg:'okay', foo:'bar'});
99+
// We don't want to disrupt the output of these types of calls, since they are can be used
100+
// for analytics...
101+
if (typeof message === 'string' && optionalParams.length <= 1) {
102+
return {
103+
type: message,
104+
data: optionalParams[0] || {},
105+
};
63106
}
107+
108+
// TODO: FXA-9951 - Will prevent this kind of call from occurring by restricting logging types.
109+
// Note, this can result in messy or invalid states in bq on jsonPayload.fields. It's
110+
// best not rely on this.
111+
return {
112+
type: '',
113+
data: {
114+
message,
115+
...optionalParams,
116+
},
117+
};
64118
}

‎libs/shared/notifier/src/lib/notifier.provider.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,20 +2,20 @@
22
* License, v. 2.0. If a copy of the MPL was not distributed with this
33
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
44

5-
import { LoggerService, Provider } from '@nestjs/common';
65
import { ConfigService } from '@nestjs/config';
7-
import { LOGGER_PROVIDER } from '@fxa/shared/log';
6+
import { Provider } from '@nestjs/common';
87
import { StatsDService } from '@fxa/shared/metrics/statsd';
98
import { SNS } from 'aws-sdk';
109
import { StatsD } from 'hot-shots';
1110
import { NotifierSnsService } from './notifier.sns.provider';
1211
import { NotifierService } from './notifier.service';
12+
import { MozLoggerService } from '@fxa/shared/mozlog';
1313

1414
export const LegacyNotifierServiceProvider: Provider<NotifierService> = {
1515
provide: NotifierService,
1616
useFactory: (
1717
configService: ConfigService,
18-
log: LoggerService,
18+
log: MozLoggerService,
1919
sns: SNS,
2020
statsd: StatsD | undefined
2121
) => {
@@ -25,5 +25,5 @@ export const LegacyNotifierServiceProvider: Provider<NotifierService> = {
2525
}
2626
return new NotifierService(config, log, sns, statsd);
2727
},
28-
inject: [ConfigService, LOGGER_PROVIDER, NotifierSnsService, StatsDService],
28+
inject: [ConfigService, MozLoggerService, NotifierSnsService, StatsDService],
2929
};

‎libs/shared/sentry/project.json‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,14 @@
1515
"outputFileName": "main.js",
1616
"tsConfig": "libs/shared/sentry/tsconfig.lib.json",
1717
"declaration": true,
18+
"external": [
19+
"@apollo/gateway",
20+
"@apollo/subgraph",
21+
"@as-integrations/fastify",
22+
"@nestjs/websockets/socket-module",
23+
"@nestjs/microservices/microservices-module",
24+
"@nestjs/microservices"
25+
],
1826
"assets": [
1927
{
2028
"glob": "libs/shared/sentry/README.md",

‎packages/fxa-admin-server/src/app.module.ts‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,13 @@
33
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
44

55
import { HealthModule } from 'fxa-shared/nestjs/health/health.module';
6-
import { LoggerModule } from 'fxa-shared/nestjs/logger/logger.module';
6+
import { MozLoggerModule, MozLoggerService } from '@fxa/shared/mozlog';
77
import { MetricsFactory } from 'fxa-shared/nestjs/metrics.service';
88
import { getVersionInfo } from 'fxa-shared/nestjs/version';
99
import { join } from 'path';
1010
import { APP_FILTER } from '@nestjs/core';
1111
import { SentryGlobalGraphQLFilter, SentryModule } from '@sentry/nestjs/setup';
12-
import { LOGGER_PROVIDER } from '@fxa/shared/log';
1312
import { LegacyStatsDProvider } from '@fxa/shared/metrics/statsd';
14-
import { MozLoggerService } from '@fxa/shared/mozlog';
1513
import {
1614
LegacyNotifierServiceProvider,
1715
LegacyNotifierSnsFactory,
@@ -31,6 +29,7 @@ import { EventLoggingModule } from './event-logging/event-logging.module';
3129
import { GqlModule } from './gql/gql.module';
3230
import { NewslettersModule } from './newsletters/newsletters.module';
3331
import { SubscriptionModule } from './subscriptions/subscriptions.module';
32+
import { LOGGER_PROVIDER } from '@fxa/shared/log';
3433

3534
const version = getVersionInfo(__dirname);
3635

@@ -71,7 +70,7 @@ const version = getVersionInfo(__dirname);
7170
extraHealthData: () => db.dbHealthCheck(),
7271
}),
7372
}),
74-
LoggerModule,
73+
MozLoggerModule,
7574
],
7675
controllers: [],
7776
providers: [
@@ -80,10 +79,6 @@ const version = getVersionInfo(__dirname);
8079
provide: APP_GUARD,
8180
useClass: UserGroupGuard,
8281
},
83-
{
84-
provide: LOGGER_PROVIDER,
85-
useClass: MozLoggerService,
86-
},
8782
LegacyNotifierServiceProvider,
8883
LegacyNotifierSnsFactory,
8984
LegacyStatsDProvider,

‎packages/fxa-admin-server/src/auth/user-group-header.guard.spec.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,19 +2,19 @@
22
* License, v. 2.0. If a copy of the MPL was not distributed with this
33
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
44

5-
import { LOGGER_PROVIDER } from '@fxa/shared/log';
5+
import { MozLoggerService } from '@fxa/shared/mozlog';
66
import { ExecutionContext, Provider } from '@nestjs/common';
77
import { Reflector } from '@nestjs/core';
88
import { Test, TestingModule } from '@nestjs/testing';
9+
import { createMock } from '@golevelup/ts-jest';
10+
911
import {
1012
AdminPanelFeature,
1113
AdminPanelGroup,
1214
USER_GROUP_HEADER,
1315
} from 'fxa-shared/guards';
1416
import { UserGroupGuard } from './user-group-header.guard';
1517

16-
import { createMock } from '@golevelup/ts-jest';
17-
1818
describe('UserGroupGuard for graphql', () => {
1919
let module: TestingModule;
2020
let guard: UserGroupGuard;
@@ -50,7 +50,7 @@ describe('UserGroupGuard for graphql', () => {
5050
beforeEach(async () => {
5151
logger = { debug: jest.fn(), error: jest.fn(), info: jest.fn() };
5252
const MockMozLogger: Provider = {
53-
provide: LOGGER_PROVIDER,
53+
provide: MozLoggerService,
5454
useValue: logger,
5555
};
5656

‎packages/fxa-admin-server/src/auth/user-group-header.guard.ts‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
* License, v. 2.0. If a copy of the MPL was not distributed with this
33
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
44

5-
import { LOGGER_PROVIDER } from '@fxa/shared/log';
65
import { MozLoggerService } from '@fxa/shared/mozlog';
76
import {
87
CanActivate,
@@ -25,10 +24,7 @@ const guard = new AdminPanelGuard(config.get('guard.env') as AdminPanelEnv);
2524

2625
@Injectable()
2726
export class UserGroupGuard implements CanActivate {
28-
constructor(
29-
private reflector?: Reflector,
30-
@Inject(LOGGER_PROVIDER) private log?: MozLoggerService
31-
) {}
27+
constructor(private reflector?: Reflector, private log?: MozLoggerService) {}
3228

3329
canActivate(context: ExecutionContext): boolean {
3430
// Reflect on the end point to determine if it has been tagged with admin panel feature.

‎packages/fxa-admin-server/src/database/database.module.ts‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,21 +2,13 @@
22
* License, v. 2.0. If a copy of the MPL was not distributed with this
33
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
44

5-
import { LOGGER_PROVIDER } from '@fxa/shared/log';
65
import { MozLoggerService } from '@fxa/shared/mozlog';
76
import { Module } from '@nestjs/common';
87
import { MetricsFactory } from 'fxa-shared/nestjs/metrics.service';
98
import { DatabaseService } from './database.service';
109

1110
@Module({
12-
providers: [
13-
DatabaseService,
14-
{
15-
provide: LOGGER_PROVIDER,
16-
useClass: MozLoggerService,
17-
},
18-
MetricsFactory,
19-
],
11+
providers: [DatabaseService, MozLoggerService, MetricsFactory],
2012
exports: [DatabaseService],
2113
})
2214
export class DatabaseModule {}

0 commit comments

Comments
 (0)