Skip to content

Added extension search text length to telemetry#148785

Merged
isidorn merged 5 commits intomicrosoft:mainfrom
prashantvc:main
May 9, 2022
Merged

Added extension search text length to telemetry#148785
isidorn merged 5 commits intomicrosoft:mainfrom
prashantvc:main

Conversation

@prashantvc
Copy link
Copy Markdown
Contributor

@prashantvc prashantvc commented May 5, 2022

This PR adds the length of search text to the telemetry

@prashantvc prashantvc marked this pull request as ready for review May 5, 2022 09:49
@isidorn
Copy link
Copy Markdown
Collaborator

isidorn commented May 5, 2022

This looks ok to me, but I leave the final review to @sandy081 as he would know best if this catches all the cases where we have the searchText .

@prashantvc did you try this out, if it catches the data you need in all cases. To build VS Code out of source just follow this doc https://github.com/microsoft/vscode/wiki/How-to-Contribute
And then you can check the telemetry output log to see if the query length is being captured as you intended.

@isidorn isidorn requested a review from sandy081 May 5, 2022 10:00
@isidorn isidorn added the extensions Issues concerning extensions label May 5, 2022
@isidorn isidorn added this to the May 2022 milestone May 5, 2022
@isidorn isidorn self-requested a review May 5, 2022 10:02
@prashantvc
Copy link
Copy Markdown
Contributor Author

And then you can check the telemetry output log to see if the query length is being captured as you intended.

Yes, I tried it locally on my machine! It seems to capture the data that I need

withSearchTextLength(length: number): Query {
return new Query({ ...this.state, searchTextLength: length });
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the right location to add data for telemetry and this method is not needed. You can add following code

searchtextLength: this.searchText.length

in the following location

and add this property in the telemetry classification here

readonly pageNumber: { classification: 'SystemMetaData'; purpose: 'FeatureInsight' };

and

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for review fixing them now

@isidorn
Copy link
Copy Markdown
Collaborator

isidorn commented May 9, 2022

@prashantvc let us know when this is good for review again. Thank you

@prashantvc
Copy link
Copy Markdown
Contributor Author

let us know when this is good for review again. Thank you

It's ready now

Copy link
Copy Markdown
Member

@sandy081 sandy081 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Thanks

@isidorn
Copy link
Copy Markdown
Collaborator

isidorn commented May 9, 2022

Looks good, thanks a lot @prashantvc
Let's merge this in.

@isidorn isidorn merged commit ec5f55d into microsoft:main May 9, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

extensions Issues concerning extensions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants