Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Better data when fail to find kernel connection #6070

Merged
merged 1 commit into from
Jun 2, 2021
Merged

Better data when fail to find kernel connection #6070

merged 1 commit into from
Jun 2, 2021

Conversation

DonJayamanne
Copy link
Contributor

No description provided.

DonJayamanne
DonJayamanne commented Jun 2, 2021
View reviewed changes
src/client/datascience/kernel-launcher/remoteKernelFinder.ts
@@ -165,13 +166,15 @@ export class RemoteKernelFinder implements IRemoteKernelFinder {
)}`
);

sendKernelListTelemetry(resource, items);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now we don't need to wait for users to select a kernel, else we only have part of the data.

DonJayamanne
DonJayamanne commented Jun 2, 2021
View reviewed changes
src/client/datascience/notebook/notebookControllerManager.ts
@@ -145,8 +145,11 @@ export class NotebookControllerManager implements INotebookControllerManager, IE
const controllers = await this.createNotebookControllers(connections);

// Send telemetry related to fetching the kernel connections
// KERNELPUSH: undefined works for telemetry?
sendNotebookControllerCreateTelemetry(undefined, controllers, stopWatch);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was incorrect, we were sending undefined, hence the resource was not a notebook, and the mapping of kernel information for the notebooks was not showing up in telemetry.

DonJayamanne
DonJayamanne commented Jun 2, 2021
View reviewed changes
src/client/datascience/raw-kernel/liveshare/hostRawNotebookProvider.ts
kernelConnectionProvided: !!kernelConnection,
notebookMetadataProvided: !!notebookMetadata,
hasKernelSpecInMetadata: !!notebookMetadata?.kernelspec,
kernelConnectionFound: !!kernelConnectionMetadata
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to find out why these errors are being throw for webview notebooks.
Its possible with the recent fixes, the number of these cases would go down, but would like to ensure we have better data in place.

DonJayamanne
DonJayamanne commented Jun 2, 2021
View reviewed changes
src/client/datascience/telemetry/kernelTelemetry.ts
sendKernelTelemetryEvent(resource, Telemetry.KernelCount, stopWatch.elapsedTime, counters);
}

export function sendNotebookControllerCreateTelemetry(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed duplicate function, no different from the other, just an argument, hence just reused.

@DonJayamanne DonJayamanne marked this pull request as ready for review June 2, 2021 17:40
@DonJayamanne DonJayamanne requested a review from a team as a code owner June 2, 2021 17:40
rchiodo
rchiodo reviewed Jun 2, 2021
View reviewed changes
src/client/datascience/notebook/notebookControllerManager.ts
// KERNELPUSH: undefined works for telemetry?
sendNotebookControllerCreateTelemetry(undefined, controllers, stopWatch);
sendKernelListTelemetry(
Uri.file('test.ipynb'), // Give a dummy ipynb value, we need this as its used in telemetry to determine the resource.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better 'if undefined use 'test.ipynb' was in the sendKernelListTelemetry? It seems like it would know better that undefined is not supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to do that, as we have plenty of places in interactive window where we end up with undefined for the resource.
I.e. this is backwards compatible, resource can be undefined & only in the case of interactive window (with notebooks we always have ipynb).

@DonJayamanne DonJayamanne merged commit f6acbbc into main Jun 2, 2021
@DonJayamanne DonJayamanne deleted the betterKernelErrorTelemetry branch June 2, 2021 18:30
DonJayamanne added a commit that referenced this pull request Jun 2, 2021
@DonJayamanne
Better data when fail to find kernel connection (#6070)
893ea7f
DavidKutu pushed a commit that referenced this pull request Jun 8, 2021
@IanMatthewHuff @DonJayamanne @joyceerhl
merge june release back (#6193)
5580aa1 
* release candidate (#6059)

* release candidate

* Memorial Day API changes (#6056)

* update changelog

Co-authored-by: Ian Huff <ianhu@microsoft.com>

* Memorial Day API changes (#6056)

* Send only error type in reason (#6067)

* Fallback for sys.prefix not being returned by Python extension (#6053)

* Fixes to toggling output (#6068)

* Better telemetry when we don't find a matching kernel (#6060)

* Better data when fail to find kernel connection (#6070)

* change version and API port (#6085)

* change version to 2021.6.x

* match packagejson and changelog versions

* 6/2 API Changes (#6089)

Co-authored-by: Ian Huff <ianhu@microsoft.com>

* Port #6025 to release (#6103)

* Remove our Run Above/Below commands (#6026)

* Update changelog

* Fix pervasive test issue with editor properties (#6100) (#6106)

* update version to 2021.6.99 to be able to filter (#6122)

* update version to 2021.6.99 to be able to filter
update changelog

* update package lock and VSCode api

* don

* remove api changes in the code

* Port Don's fixes (#6115)

* Fix test failures resulting from VSCodes Notebook Start page (#6111)

* Disable kernel auto startup in untrusted workspace (#6088)

* Disable kernel auto startup in untrusted workspace

* Fixes

* Misc

* oops

* misc

* Fixes to breaking tests (#6074)

Co-authored-by: Don Jayamanne <don.jayamanne@outlook.com>

* Breaking changes ports to release (2 commits) (#6140)

* add component governance file (#6166)

* add component governance file

* remove npm components

* Cherry pick changes from main branch into release (#6174)

* final update (#6176)

* final update

* update changelog

* update verion, engine
revert change on gulpfile

* merge main

* revert engine to 1.57-insider

* delete news files that are on the changelog

* disable insiders build

Co-authored-by: Ian Huff <ianhu@microsoft.com>
Co-authored-by: Don Jayamanne <don.jayamanne@yahoo.com>
Co-authored-by: Don Jayamanne <don.jayamanne@outlook.com>
Co-authored-by: Joyce Er <joyceerhuiling@gmail.com>
DavidKutu pushed a commit that referenced this pull request Jun 10, 2021
@IanMatthewHuff @DonJayamanne @joyceerhl
Merge June release back to main (#6228)
aa7f583 
* release candidate (#6059)

* release candidate

* Memorial Day API changes (#6056)

* update changelog

Co-authored-by: Ian Huff <ianhu@microsoft.com>

* Memorial Day API changes (#6056)

* Send only error type in reason (#6067)

* Fallback for sys.prefix not being returned by Python extension (#6053)

* Fixes to toggling output (#6068)

* Better telemetry when we don't find a matching kernel (#6060)

* Better data when fail to find kernel connection (#6070)

* change version and API port (#6085)

* change version to 2021.6.x

* match packagejson and changelog versions

* 6/2 API Changes (#6089)

Co-authored-by: Ian Huff <ianhu@microsoft.com>

* Port #6025 to release (#6103)

* Remove our Run Above/Below commands (#6026)

* Update changelog

* Fix pervasive test issue with editor properties (#6100) (#6106)

* update version to 2021.6.99 to be able to filter (#6122)

* update version to 2021.6.99 to be able to filter
update changelog

* update package lock and VSCode api

* don

* remove api changes in the code

* Port Don's fixes (#6115)

* Fix test failures resulting from VSCodes Notebook Start page (#6111)

* Disable kernel auto startup in untrusted workspace (#6088)

* Disable kernel auto startup in untrusted workspace

* Fixes

* Misc

* oops

* misc

* Fixes to breaking tests (#6074)

Co-authored-by: Don Jayamanne <don.jayamanne@outlook.com>

* Breaking changes ports to release (2 commits) (#6140)

* add component governance file (#6166)

* add component governance file

* remove npm components

* Cherry pick changes from main branch into release (#6174)

* final update (#6176)

* final update

* update changelog

* add missing thanks to changelog (#6197)

* Skip uploading vsix to azure blob store (#6204)

* publish release

* publish release

* remove last 2 numbers from release (#6219)

* publish release

* fix conflicts

* update engine to 1.58.0-insider

* return engine version to 1.57.0-insider

* Do not activate Python before opening nb (#6201) (#6230)

* publish release

Co-authored-by: Ian Huff <ianhu@microsoft.com>
Co-authored-by: Don Jayamanne <don.jayamanne@yahoo.com>
Co-authored-by: Don Jayamanne <don.jayamanne@outlook.com>
Co-authored-by: Joyce Er <joyceerhuiling@gmail.com>
DavidKutu pushed a commit that referenced this pull request Jun 16, 2021
@IanMatthewHuff @DonJayamanne @joyceerhl
point release back to main (#6301)
3fd9151 
* release candidate (#6059)

* release candidate

* Memorial Day API changes (#6056)

* update changelog

Co-authored-by: Ian Huff <ianhu@microsoft.com>

* Memorial Day API changes (#6056)

* Send only error type in reason (#6067)

* Fallback for sys.prefix not being returned by Python extension (#6053)

* Fixes to toggling output (#6068)

* Better telemetry when we don't find a matching kernel (#6060)

* Better data when fail to find kernel connection (#6070)

* change version and API port (#6085)

* change version to 2021.6.x

* match packagejson and changelog versions

* 6/2 API Changes (#6089)

Co-authored-by: Ian Huff <ianhu@microsoft.com>

* Port #6025 to release (#6103)

* Remove our Run Above/Below commands (#6026)

* Update changelog

* Fix pervasive test issue with editor properties (#6100) (#6106)

* update version to 2021.6.99 to be able to filter (#6122)

* update version to 2021.6.99 to be able to filter
update changelog

* update package lock and VSCode api

* don

* remove api changes in the code

* Port Don's fixes (#6115)

* Fix test failures resulting from VSCodes Notebook Start page (#6111)

* Disable kernel auto startup in untrusted workspace (#6088)

* Disable kernel auto startup in untrusted workspace

* Fixes

* Misc

* oops

* misc

* Fixes to breaking tests (#6074)

Co-authored-by: Don Jayamanne <don.jayamanne@outlook.com>

* Breaking changes ports to release (2 commits) (#6140)

* add component governance file (#6166)

* add component governance file

* remove npm components

* Cherry pick changes from main branch into release (#6174)

* final update (#6176)

* final update

* update changelog

* add missing thanks to changelog (#6197)

* Skip uploading vsix to azure blob store (#6204)

* publish release

* publish release

* remove last 2 numbers from release (#6219)

* publish release

* Do not activate Python before opening nb (#6201) (#6230)

* publish release

* Port LiveKernelModel fix to release for point release (#6264)

* Port keybinding updates to release (#6268)

* Contribute shift+enter, ctrl+enter, L, shift+L (#6205)

* Ctrl+Enter should put cell into command mode after executing (#6231)

* Update CHANGELOG and remove news

* port test fix (#6275)

* Fix Restarting kernel... test (#6267)

* wait for the restart command

* add news file

* update changelog

* Fix native notebook interrupt toolbar (#6280) (#6282)

* update changelog (#6284)

* -update version
-update changelog

* leave version as it was

* Respect jupyter.enableKeyboardShortcuts setting and enable ctrl+enter in command mode (#6293)

* publish release

* fix gulpfile

* undo change

Co-authored-by: Ian Huff <ianhu@microsoft.com>
Co-authored-by: Don Jayamanne <don.jayamanne@yahoo.com>
Co-authored-by: Don Jayamanne <don.jayamanne@outlook.com>
Co-authored-by: Joyce Er <joyceerhuiling@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rchiodo rchiodo rchiodo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DonJayamanne @rchiodo