Skip to content

Az.Tools.Predictor refactor and performance improvement. #13669

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

Merged
merged 15 commits into from
Dec 15, 2020

Conversation

kceiw
Copy link
Contributor

@kceiw kceiw commented Dec 4, 2020

Description

There are a few things going on during the refactor: telemetry, readability, and performance.

  1. Replace Tuple/ValueTuple with more meaningful types that don't use 'Item1' and 'Item2'.
  2. Replace Newtonsoft Json library with the built-in System.Text.Json.
  3. Reformat the telemetry events. Combine the error events with the normal event. Just have a field in the event to indicate if there is an exception or not.
  4. Collect the event if the http request to the server has been sent or not. The content isn't collected.
  5. Transform data format and send telemetry event in a worker in the default thread pool. This can also improve the performance since that removes the burden on the thread that's handling callback from PSReadLine.
  6. Validate parameter values and also catch http response error early instead of when we try to parse the response content.
  7. Remove duplicate parsing on the user input.
  8. Reduce string manipulation.

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
      • {Please put the link here}
    • the markdown help files have been regenerated using the commands listed here

kceiw added 14 commits November 30, 2020 15:42
- Improve the comment and its format.
- Create a concret class type to replace Tuple and ValueTuple.
- Verify method parameter values.
- Combine the error telemetry event with the non-error one.
- Collect if the http request is canceled when we send http request.
- Refactor the telemetry and use a class for the collected data in each
  telemetry event.
- Now we only get basic information and push them to the data flow.
- A thread from thread pool handles the data, transform them, and send
  it.
- Updated the test after the refactor.
- Add more test cases.
- We don't set SuggestionSource on the suggestion in some cases. This is
  revealed in the unit tests. They're fixed.
- We have two CommandLinePredictor in AzurePredictorService. The
  CommandLinePredictor needs to extract from the user input the command
  name, parameter set etc. It's duplicate if we do that in both
  CommandLinePredictor. Move that extraction to AzurePredictorService
  and the CommandLinePredictor will not need to do it.
- Remove the string manipulation.
- Pre-allocate the collections for the result.
- Remove invariant check in "readonly" properties.
@kceiw kceiw requested a review from mirdaki December 10, 2020 17:31
@mirdaki
Copy link

mirdaki commented Dec 11, 2020

Looks good to me!

/// <returns>true if allowed.</returns>
private static bool IsDataCollectionAllowed()
{
if (AzurePSDataCollectionProfile.Instance.EnableAzureDataCollection == true)

Choose a reason for hiding this comment

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

nit: just return AzurePSDataCollectionProfile.Instance.EnableAzureDataCollection

Copy link

@baopingz baopingz left a comment

Choose a reason for hiding this comment

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

looks good.

@dingmeng-xue dingmeng-xue merged commit 91f1b75 into Azure:master Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants