Initialize transport service in Performance Monitoring when performance() is called. #2506
Conversation
zijianjoy
commented
Jan 8, 2020
zijianjoy
commented
- Rename cc_service to transport_service since we expect migration to Firelog service in the future.
- transport_service.ts used to start setTimeout() when this file is loaded. This change wraps it into initialization function, which should be called when performance() is called (it is when developer activates Performance Monitoring component).
- Update testing to call useFakeTimers() inside describe() instead of before loading transport_service.ts.
There was a problem hiding this comment.
Tests look great, thanks!
One nit, please feel free to ignore if this is out of scope, but there are some ongoing TS errors (from before this change) on oob_resources_services.test.ts, would it be possible to add //@ts-ignore Assignment to read-only property. lines above the two assignments to api.onFirstInputDelay in that file, to get rid of the console errors when running perf tests?
| @@ -17,7 +17,7 @@ | |||
|
|
|||
| import { stub, SinonStub, useFakeTimers, SinonFakeTimers } from 'sinon'; | |||
| import { Trace } from '../resources/trace'; | |||
| import * as ccService from './cc_service'; | |||
| import * as ccService from './transport_service'; | |||
There was a problem hiding this comment.
Since we are replacing all the context of CC to Transport, should we import it as transportService instead of ccService?
There was a problem hiding this comment.
Updated, thank you!
| fetchStub.restore(); | ||
| }); | ||
|
|
||
| it('throws an error when logging an empty message', done => { |
There was a problem hiding this comment.
I do not think the "done" is doing much here. It is usually used for callback based async code. Here adding a done at the end of the main block does not accomplish much.
https://mochajs.org/#asynchronous-code
There was a problem hiding this comment.
Removed for not async test.
| @@ -27,6 +28,7 @@ export function setupOobResources(): void { | |||
| if (!getIid()) { | |||
| return; | |||
| } | |||
| setupTransportService(); | |||
There was a problem hiding this comment.
setupOobResources is called when the call to get iid and RC is responsed. Starting the transport cycle here means that the initial time of sending logs is "iid call + RC call + 5.5s". I suggest not including those calls in the initial delay to avoid it from being undeterministic.
There was a problem hiding this comment.
Moved initialization call to controllers/perf.ts.
