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.

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

Adding telemetry for debugger time-to-start performance #7323

Merged
merged 2 commits into from
Sep 13, 2019

Conversation

karthiknadig
Copy link
Member

@karthiknadig karthiknadig commented Sep 11, 2019

For #7332

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Has a news entry file (remember to thank yourself!)
  • Appropriate comments and documentation strings in the code
    - [ ] Has sufficient logging.
  • Has telemetry for enhancements.
    - [ ] Unit tests & system/integration tests are added/updated
    - [ ] Test plan is updated as appropriate
    - [ ] package-lock.json has been regenerated by running npm install (if dependencies have changed)
    - [ ] The wiki is updated with any design decisions/details.

@karthiknadig
Copy link
Member Author

@luabud is there work item for this in the extension repo? I can create one if not. I will update that and the news item after.

@codecov-io
Copy link

codecov-io commented Sep 11, 2019

Codecov Report

Merging #7323 into master will decrease coverage by 0.06%.
The diff coverage is 47.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7323      +/-   ##
==========================================
- Coverage   58.36%   58.29%   -0.07%     
==========================================
  Files         493      493              
  Lines       21667    21690      +23     
  Branches     3486     3489       +3     
==========================================
  Hits        12645    12645              
- Misses       8242     8266      +24     
+ Partials      780      779       -1
Impacted Files Coverage Δ
src/client/telemetry/index.ts 86.13% <ø> (ø) ⬆️
src/client/debugger/types.ts 100% <ø> (ø) ⬆️
src/client/telemetry/constants.ts 100% <100%> (ø) ⬆️
src/client/datascience/debugLocationTracker.ts 69.56% <36.84%> (-12.44%) ⬇️
src/client/common/logger.ts 68.55% <0%> (-6.92%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7362b25...d4962c6. Read the comment docs.

@kimadeline kimadeline added no-changelog No news entry required and removed no-changelog No news entry required labels Sep 11, 2019
src/client/datascience/debugLocationTracker.ts Outdated Show resolved Hide resolved
src/client/datascience/debugLocationTracker.ts Outdated Show resolved Hide resolved
@kimadeline
Copy link

I approved the DS changes, but there are still extension changes pending 🤦‍♀

Copy link

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

This seems to measure just the data science debugger, not the standard debugger user by extension!
Shouldn't we do it for both, in a more generic way? I.e. benefit everyone.
This way we can add more telemetry in the future for other bits as well.

Also the class modified is named debugLocationTracker, doesn't feel like the right place to add this perf stuff.
I'd prefer if we created a separate tracker class for perf, and moved it out of data science folder to make it generic to the entire Extension.

@karthiknadig
Copy link
Member Author

@DonJayamanne You can register only one tracker per language. This runs for anytime a debug configuration has language python. Let me know if you have any suggestion on reorganizing this.

@karthiknadig
Copy link
Member Author

Note that there are a bunch of issues with the tracker here.

  1. The DS tracker does not isolate itself from non-DS scenarios. This tracker is used in all debug sessions where the language is python.
  2. The DS tracker factory reuses a single tracker instance. The debug session is updated on it when ever a new session starts. This breaks in multi proc scenario where there can be multiple simultaneous debug sessions.

Let me know if I can go ahead and make the change to create a global tracker for python and let DS register itself with that. The DA can register telemetry with that too. Essentially observer pattern.

@karthiknadig
Copy link
Member Author

Talked to @kimadeline created a separate item to track the refactor of this tracker https://github.com/microsoft/vscode-python/issues/7352

@karthiknadig karthiknadig merged commit b9ce735 into microsoft:master Sep 13, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 20, 2019
@karthiknadig karthiknadig deleted the telemetry branch November 1, 2019 17:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants