Introduces a telemetry collector. #314
Conversation
| @@ -179,7 +181,7 @@ export interface IDebugAdapter { | |||
| shutdown(): void; | |||
|
|
|||
| initialize(args: DebugProtocol.InitializeRequestArguments, requestSeq?: number): PromiseOrNot<DebugProtocol.Capabilities>; | |||
| launch(args: ILaunchRequestArgs, requestSeq?: number): PromiseOrNot<void>; | |||
| launch(args: ILaunchRequestArgs, telemetryPropertyCollector: ITelemetryPropertyCollector, requestSeq?: number): PromiseOrNot<void>; | |||
There was a problem hiding this comment.
After I get some consensus about this design. I can modify all the request methods as well.
| @@ -124,8 +124,9 @@ export class ChromeDebugSession extends LoggingDebugSession implements IObservab | |||
| * Overload dispatchRequest to the debug adapters' Promise-based methods instead of DebugSession's callback-based methods | |||
| */ | |||
| protected dispatchRequest(request: DebugProtocol.Request): void { | |||
| const telemetryPropertyCollector = new TelemetryPropertyCollector(); | |||
There was a problem hiding this comment.
What are the advantages of creating the TelemetryPropertyCollector here instead of receiving it from the reportTelemetry method?
There was a problem hiding this comment.
I think they are equivalent.
There was a problem hiding this comment.
What do you think would make the code simpler and easier to maintain?
A. Keeping as much of the telemetry implementation inside the reportTelemetry method, and having reportTelemetry create the telemetryPropertyCollector so that the business methods don't need to deal with details of the telemetry implementation?
B. Spreading the telemetry implementation between reportTelemetry and the business methods, and having each business method that uses telemetry having to create it's own telemetryPropertyCollector?
…end additional properties reported in the request telemetry. # Conflicts: # src/chrome/chromeDebugAdapter.ts # src/chrome/chromeDebugSession.ts
|
What extra properties will this be used for? |
|
@roblourens This will be highly re-used for some notification handlers. Say when a breakpoint is hit, we need to know whether is a real or pesudo(break-on-load) breakpoint, whether it's for JS or TS. Currently our telemetry facility can only measure a processing time, successfulness etc. It does not support adding customized properties per the business of each handler. |
|
I don't understand, since this is hooked up for client requests, how would you use it for notifications like onPaused? I think it makes sense for client requests but for cases where ChromeDebugAdapter fires the telemetry events itself, it could just pass in any extra properties to reportEvent that it needs to, right? |
|
You are right. We haven't implemented that for notifications. This PR is just an example showing how a TelemetryCollector can be used. For OnPaused. since we already has a wrapper to measure processing time in a universal way like this: (The method will be refactored in a similar fashion） The |
so a request handling logic can append additional properties reported in the request telemetry.
Currently there is no easy way for a request handler to report additional properties other than the ones universally added, as success state, time spent etc.
Returning additional fields in the return value might compromise the current semantic of the return value of each request. So I introduced an additional parameter to receive the additional telemetry properties.
This collector can also be used in the handlers for the notifications as needed.