[wip] az feedback #8829

adewaleo · 2019-03-20T08:58:50Z

az feedback now:

directs users to helpful links
shows last 9 commands
prompts user to create an issue template based on selected command
directs user to core repo or extensions repo based on command

Todo:

Update az login webpage to contain getting started links / commands.
Properly handle showing feedback commands (possibly duplicate commands). For now, we simply don't add az feedback to the list of recent commands.

closes #2048

This checklist is used to make sure that common guidelines for a pull request are followed.

The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).
I adhere to the Command Guidelines.

adewaleo · 2019-03-26T21:49:58Z

Please feel free to review away!

cc: @tjprescott, @yugangw-msft, @achandmsft.

adewaleo · 2019-03-28T03:33:21Z

ping! (for review)
Also not sure why the Azure Pipelines test is failing
CC: @yugangw-msft @tjprescott @marstr

marstr · 2019-03-28T16:24:16Z

src/azure-cli-core/setup.py

-    'azure-mgmt-resource==2.1.0'
+    'azure-mgmt-resource==2.1.0',
+    'pyperclip==1.7.0',
+    'psutil==5.6.1'


Looks like this dependency is causing our ADO check to fail. I suspect that this dependency isn't playing nice inside of alpine.

marstr · 2019-03-28T16:27:05Z

src/azure-cli-core/setup.py

@@ -72,7 +72,9 @@
    'six',
    'tabulate>=0.7.7',
    'wheel',
-    'azure-mgmt-resource==2.1.0'
+    'azure-mgmt-resource==2.1.0',


In general, our setup.py files should contain a range of acceptable versions, instead of one pinned version. Not doing this causes issues like #6973.

makes sense!

src/azure-cli-core/azure/cli/core/util.py

tjprescott

Privacy is my primary concern with this, but there are others as well.

tjprescott · 2019-03-26T22:05:45Z

src/azure-cli-core/azure/cli/core/azlogging.py

+
+
+UNKNOWN_COMMAND = "unknown_command"
+CMD_LOG_LINE_PREFIX = "CMD-LOG-LINE-BEGIN"


These should not be public.

tjprescott · 2019-03-28T15:00:28Z

.github/ISSUE_TEMPLATE/Bug_report.md

@@ -4,7 +4,7 @@ about: Create a report to help us improve

 ---

-**Describe the bug**
+q**Describe the bug**


yes, I noticed this too, its a typo, mistake, will update it with my next round of changes.

tjprescott · 2019-03-28T16:58:43Z

src/azure-cli-core/azure/cli/core/azlogging.py

+                    try:
+                        os.remove(os.path.join(log_dir, file))
+                    except OSError:  # FileNotFoundError introduced in Python 3
+                        continue


So this... keeps the number of files between 5 and 25?

keeps files between 0 and 20 / 25. I could rename the variable to clarify that the files are in reverse order. I wanted to make removing the files an infrequent operation that removes a bulk of unnecessary commands. Maybe I could fine tune this.

Add comments so we understand the logic.

@tjprescott, you were right, there was a bug. I have fixed it.

tjprescott · 2019-03-28T17:00:39Z

src/azure-cli-core/azure/cli/core/util.py

+        issue_url = "https://aka.ms/azcli/ext/issues" if is_extension else "https://aka.ms/azcli/issues"
+        logger.error("The command failed with an unexpected error. Here is the traceback:\n")
+        logger.exception(ex)
+        logger.warning("\nTo open an issue, please visit: %s", issue_url)


Shouldn't we direct people to use az feedback here?

We most definitely should!

tjprescott · 2019-03-28T17:02:04Z

src/azure-cli/azure/cli/__main__.py

@@ -43,9 +45,21 @@ def cli_main(cli, args):
    else:
        telemetry.set_success()

+    elapsed_time = timeit.default_timer() - start_time


Why do we log the time for each command? I worry this may attract more perf related issues being raised.

I think this was brought up in the meeting with @yugangw-msft and @achandmsft. I think the rational was to help customers notice / point out situations were commands are much slower than usual. I suppose we just saw it as general command related information.

If perf really is a problem (esp. on the CLI side) moving forward then I think it doesn't hurt to give customers and ourselves more clarity regarding it. If it really is a problem then we eventually have to address it.

I guess we could discuss this more when I am back from the conference.

tjprescott · 2019-03-28T17:06:01Z

src/command_modules/azure-cli-feedback/azure/cli/command_modules/feedback/custom.py

+
+_ISSUES_TEMPLATE = """
+
+### **This autogenerated template contains command parameters. Please review and remove sensitive information, if necessary**


Why don't we simply remove the values and replace them with placeholders? This is the kind of thing we do not want to screw up. As soon as someone DOESN'T read this and posts sensitive information (which would take all of oh... 1 week?) we have a problem.

Yes this is a tough / nuanced decision to make. On the one hand, we risk customer security by showing command arguments in the output by default, but this would often contain very helpful information.

I suppose place holders could be the happy medium. I can make this change then.

cc: @yugangw-msft @achandmsft

Well, it would be helpful to have all the data in telemetry as well... problem is, that would be illegal. :)

fair enough, lol. I will confirm with team and go ahead to replace with placeholders after scrum...

tjprescott · 2019-03-28T17:06:26Z

src/command_modules/azure-cli-feedback/azure/cli/command_modules/feedback/custom.py

+
+
+**Command Duration**
+    {command_duration}


Why is this useful?

It's just metadata about the command which could possible be useful. I suppose it doesn't hurt.

cc: @yugangw-msft @achandmsft

tjprescott · 2019-03-28T17:07:58Z

src/command_modules/azure-cli-feedback/azure/cli/command_modules/feedback/custom.py

+    `{command_name}`
+
+**Ran:**
+    `{executed_command}`


It is safer to put sanitized values here and invite the user to put actual values than vice-versa. @JasonRShaver @yugangw-msft you know more about the risks here as they are largely the same as the risks involved with telemetry.

tjprescott · 2019-03-28T17:11:18Z

src/command_modules/azure-cli-feedback/azure/cli/command_modules/feedback/custom.py

+{errors_string}
+
+## To Reproduce:
+


It seems like "Ran" from above could be combined here. At a minimum, the command they ran is the last step of the repro...

That was the goal, but @yugangw-msft suggested moving "ran..." up near the top of the issue so users are less likely to post the issue without checking the command's arguments for sensitive info.

However, if we go with the placeholder approach, then I could move this command info back down to this section..

tjprescott · 2019-03-28T17:12:10Z

src/command_modules/azure-cli-feedback/azure/cli/command_modules/feedback/custom.py

+{cli_version}
+
+## Additional context
+Add any other context about the problem here.


I would move all the stuff they would be expected to fill in manually to the top of the auto-gen template. Otherwise they will simply expect that everything is pre-filled for them.

…his metadata. Command duration and time since last run now properly shown. Print format updates. Minor bug fixes. Prompt user for issue they want info on. Fix bugs. changes with browser Skip feedback commands. Browser open works. Verbose shows issue template. Refactored code, although printing and time logic seems to be incorrect.

Delete last 5 commands if number of cached commands exceeds 25. Address more style issues. style fixes Move feedback test to correct location. More style fixes.

More style fixes. Fix python 2 error.

…o command metadata logs. Minor changes.

…back.

…. Minor updates. Fix bug where command_data_dict could be None.

adewaleo · 2019-04-02T22:13:28Z

src/azure-cli-core/azure/cli/core/util.py

-    logger.error("The command failed with an unexpected error. Here is the traceback:\n")
-    logger.exception(ex)
-    logger.warning("\nTo open an issue, please visit: %s", issue_url)
+        if isinstance(ex, (CLIError, CloudError)):


this section should be exactly the same, except that we use the command logger context and we no longer print the issues url but direct users to use az feedback.

marstr · 2019-04-02T22:57:31Z

Dockerfile

@@ -34,7 +34,7 @@ LABEL maintainer="Microsoft" \
 # pip wheel - required for CLI packaging
 # jmespath-terminal - we include jpterm as a useful tool
 RUN apk add --no-cache bash openssh ca-certificates jq curl openssl git zip \
- && apk add --no-cache --virtual .build-deps gcc make openssl-dev libffi-dev musl-dev \
+ && apk add --no-cache --virtual .build-deps gcc make openssl-dev libffi-dev musl-dev linux-headers \


Could you point me to where you found this secret-sauce? It looks promising (seeing as we haven't historically been too sensitive to image size) but I want to make sure that I understand the consequences of adding this dependency.

@marstr, this is the github issue / comment that helped me out.

tjprescott · 2019-04-04T19:16:13Z

@marstr ping.

adewaleo added the do-not-merge label Mar 20, 2019

adewaleo requested review from yugangw-msft and achandmsft March 20, 2019 08:58

adewaleo removed the do-not-merge label Mar 26, 2019

adewaleo added this to the Sprint 58 milestone Mar 26, 2019

adewaleo requested review from tjprescott and marstr March 26, 2019 04:50

adewaleo force-pushed the az-feedback-update branch 3 times, most recently from 2a9c14c to 43e0265 Compare March 26, 2019 16:05

adewaleo requested review from samkreter and removed request for samkreter March 26, 2019 19:37

adewaleo force-pushed the az-feedback-update branch from 4c14c18 to 5a0ff33 Compare March 27, 2019 19:17

marstr suggested changes Mar 28, 2019

View reviewed changes

tjprescott suggested changes Mar 28, 2019

View reviewed changes

adewaleo force-pushed the az-feedback-update branch from 0996643 to 1fb02c3 Compare April 2, 2019 20:21

Oluwatosin Adewale added 8 commits April 2, 2019 15:08

Add tests for az feedback. Update bug_report.md file. Fix exit code bug.

3019d1c

Address linter/style errors. Update History.rst

bdb8e92

Delete last 5 commands if number of cached commands exceeds 25. Address more style issues. style fixes Move feedback test to correct location. More style fixes.

Rename FileNotFoundError to OSError. Address style problems.

89dbd85

More style fixes. Fix python 2 error.

Update command logger context. Ensure that failed show commands log t…

a08113b

…o command metadata logs. Minor changes.

Fix docker build issue. Properly detect powershell. Address some feed…

0598b71

…back.

Fixed log cleanup bug. Remove sensitive information from command_args…

a840d12

…. Minor updates. Fix bug where command_data_dict could be None.

Detect powershell. Take 2.

045eede

adewaleo force-pushed the az-feedback-update branch from 50a8b9f to 045eede Compare April 2, 2019 22:09

adewaleo commented Apr 2, 2019

View reviewed changes

Fix setup.py

4429056

marstr reviewed Apr 2, 2019

View reviewed changes

Oluwatosin Adewale and others added 4 commits April 2, 2019 17:01

Style fix.

74a8da0

az feedback no longer shows command duration. Other stylistic updates.

a495d80

Style fix.

ae8829b

Pylint, line too long.

91f8623

tjprescott approved these changes Apr 4, 2019

View reviewed changes

marstr approved these changes Apr 4, 2019

View reviewed changes

adewaleo merged commit 1bab798 into Azure:dev Apr 4, 2019

jiasli mentioned this pull request Mar 16, 2021

{Log} Adapt az_command_data_logger to Knack 0.8.0 #17324

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wip] az feedback #8829

[wip] az feedback #8829

adewaleo commented Mar 20, 2019 •

edited

Loading

adewaleo commented Mar 26, 2019

adewaleo commented Mar 28, 2019

marstr Mar 28, 2019

marstr Mar 28, 2019

adewaleo Mar 29, 2019 •

edited

Loading

tjprescott left a comment

tjprescott Mar 26, 2019

tjprescott Mar 28, 2019

adewaleo Mar 29, 2019

tjprescott Mar 28, 2019

adewaleo Mar 29, 2019

tjprescott Mar 29, 2019

adewaleo Apr 2, 2019

tjprescott Mar 28, 2019

adewaleo Mar 29, 2019

tjprescott Mar 28, 2019

adewaleo Mar 29, 2019

tjprescott Mar 28, 2019

adewaleo Mar 29, 2019

tjprescott Mar 29, 2019

adewaleo Apr 1, 2019

tjprescott Mar 28, 2019

adewaleo Mar 29, 2019

tjprescott Mar 28, 2019

tjprescott Mar 28, 2019

adewaleo Mar 29, 2019

tjprescott Mar 28, 2019

adewaleo Apr 2, 2019

marstr Apr 2, 2019

adewaleo Apr 2, 2019

tjprescott commented Apr 4, 2019



		UNKNOWN_COMMAND = "unknown_command"
		CMD_LOG_LINE_PREFIX = "CMD-LOG-LINE-BEGIN"


		_ISSUES_TEMPLATE = """

		### This autogenerated template contains command parameters. Please review and remove sensitive information, if necessary

		{errors_string}

		## To Reproduce:

[wip] az feedback #8829

[wip] az feedback #8829

Conversation

adewaleo commented Mar 20, 2019 • edited Loading

adewaleo commented Mar 26, 2019

adewaleo commented Mar 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adewaleo Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

tjprescott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjprescott commented Apr 4, 2019

adewaleo commented Mar 20, 2019 •

edited

Loading

adewaleo Mar 29, 2019 •

edited

Loading