New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduces a telemetry collector. #314

Merged
merged 1 commit into from Mar 23, 2018

Conversation

Projects
None yet
3 participants
@changsi-an
Copy link
Member

changsi-an commented Mar 22, 2018

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.

@changsi-an changsi-an requested review from roblourens , rakatyal and digeff Mar 22, 2018

@changsi-an changsi-an force-pushed the changsi-an:master branch from ddaf03b to 73b0f2d Mar 23, 2018

@@ -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>;

This comment has been minimized.

@changsi-an

changsi-an Mar 23, 2018

Member

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();

This comment has been minimized.

@digeff

digeff Mar 23, 2018

Contributor

What are the advantages of creating the TelemetryPropertyCollector here instead of receiving it from the reportTelemetry method?

This comment has been minimized.

@changsi-an

changsi-an Mar 23, 2018

Member

I think they are equivalent.

This comment has been minimized.

@digeff

digeff Mar 23, 2018

Contributor

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?

Introduces a telemetry collector, so a request handling logic can app…
…end additional properties reported in the request telemetry.

# Conflicts:
#	src/chrome/chromeDebugAdapter.ts
#	src/chrome/chromeDebugSession.ts

@changsi-an changsi-an force-pushed the changsi-an:master branch from 73b0f2d to bfc8fb4 Mar 23, 2018

@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Mar 23, 2018

What extra properties will this be used for?

@changsi-an

This comment has been minimized.

Copy link
Member

changsi-an commented Mar 23, 2018

@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.

@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Mar 23, 2018

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?

@changsi-an

This comment has been minimized.

Copy link
Member

changsi-an commented Mar 23, 2018

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:

            this.runAndMeasureProcessingTime('target/notification/onPaused', () => {
                return this.onPaused(params);
            });

(The method will be refactored in a similar fashion)

The runAndMeasureProcessingTime method will create a telemetry entry to describe what/how the onPause handler does. It is true that we can have onPause call another telemetry.reportEvent(), but that would become a new telemetry entry. We want to merge everything into the one emitted by runAndMeasureProcessingTime, so we don't have to join/correlate telemetry entries down the pipeline.

@changsi-an changsi-an merged commit bfc8fb4 into Microsoft:master Mar 23, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@roblourens roblourens added this to the March 2018 milestone Apr 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment