Skip to content

Capture request timings in language server client middleware#10419

Merged
jakebailey merged 7 commits into
microsoft:masterfrom
jakebailey:ls-telem
Mar 4, 2020
Merged

Capture request timings in language server client middleware#10419
jakebailey merged 7 commits into
microsoft:masterfrom
jakebailey:ls-telem

Conversation

@jakebailey

@jakebailey jakebailey commented Mar 3, 2020

Copy link
Copy Markdown
Member

For #10392.

I'd like some feedback on some of the numbers I've chosen here. I'm using the AB test mechanism to help limit flow for the non-node LS, so it's possible I can lax some of the other limits I have.

  • 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!). Marked as skip-news. Can make an enhancement w/ news if wanted.
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated. Tentatively hoping this isn't needed.
  • 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.

@jakebailey jakebailey added the no-changelog No news entry required label Mar 3, 2020
@codecov-io

codecov-io commented Mar 3, 2020

Copy link
Copy Markdown

Codecov Report

Merging #10419 into master will increase coverage by <.01%.
The diff coverage is 53.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10419      +/-   ##
==========================================
+ Coverage   60.94%   60.94%   +<.01%     
==========================================
  Files         578      578              
  Lines       31121    31188      +67     
  Branches     4418     4428      +10     
==========================================
+ Hits        18966    19008      +42     
- Misses      11194    11215      +21     
- Partials      961      965       +4
Impacted Files Coverage Δ
src/client/telemetry/index.ts 87.38% <ø> (ø) ⬆️
src/client/activation/languageServer/manager.ts 83.6% <100%> (+0.84%) ⬆️
src/client/telemetry/constants.ts 100% <100%> (ø) ⬆️
src/client/common/experimentGroups.ts 100% <100%> (ø) ⬆️
src/client/activation/node/manager.ts 28.3% <28.57%> (-1.7%) ⬇️
src/client/activation/languageClientMiddleware.ts 33.59% <45.45%> (+25.48%) ⬆️
src/datascience-ui/react-common/arePathsSame.ts 75% <0%> (-12.5%) ⬇️
src/client/common/utils/platform.ts 64.7% <0%> (-11.77%) ⬇️
src/client/linters/pydocstyle.ts 86.66% <0%> (-2.23%) ⬇️
src/client/datascience/debugLocationTracker.ts 76.56% <0%> (-1.57%) ⬇️
... and 2 more

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 888ff0f...0f0a09d. Read the comment docs.

@jakebailey

Copy link
Copy Markdown
Member Author

Hold off on merging, I'm looking into cleaning up some of my duplication.

@sonarqubecloud

sonarqubecloud Bot commented Mar 4, 2020

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jakebailey

Copy link
Copy Markdown
Member Author

@DonJayamanne Thanks for looking. I shifted things around a little, but the general scheme is about the same.

No clue why CI keeps timing out...

@DonJayamanne

Copy link
Copy Markdown

You can ignore the timeout, it's a known issue

@jakebailey
jakebailey merged commit 5ab1b73 into microsoft:master Mar 4, 2020
@jakebailey
jakebailey deleted the ls-telem branch March 4, 2020 23:31
@lock lock Bot locked as resolved and limited conversation to collaborators Mar 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

no-changelog No news entry required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants