-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Azure Monitor cli commands for Managemnt & Data plane #1953
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be a good time to move this utility class into azure.cli.core
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. If done, it should be a separate PR to not make this review more difficult.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Then I'd prefer it to refactor once this PR gets in.
|
You need to fix the linter issuer (add "# pylint: skip-file" to any auto-genned SDK) and also add a comment to the PR with the -h output for the most important commands. |
tjprescott
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to see help output for the new commands before approving. Otherwise just some minor things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should go in a _client_factory.py file to be consistent with other packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still think the client factories should be in _client_factory.py file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. If done, it should be a separate PR to not make this review more difficult.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No other command mentions a "REST API" in the CLI that I know of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya that's true..that came from swagger comment...I'll remove it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated: Update a log profile assigned to Azure subscription
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: metrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might make sense to clarify "manage Monitor alert rules" etc. for these commands since the terms are pretty generic and could be confused for other things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated: Commands to manage alerts assigned to Azure resources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The --name parameters here need a values completer within their scope (except for create commands).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--azure-resource-name is pretty verbose. Also, will this accept a name or id? Should it? If so, I would recommend --resource with help text that says "Name or ID of the resource to query." or something to that effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't done tests on autoscale, so i am not sure on whether it can accept ids. So I'll keep this pending until then :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend you spell out the kwargs like we do in the traditional model. (i.e. type=json.loads, help='blahblah')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool
src/command_modules/azure-cli-monitor/azure/cli/command_modules/monitor/validators.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this line used for?
It shouldn't be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a formality though it is not technically needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another way of just doing modpath.format(a, b, c) like in the VM module (https://github.com/Azure/azure-cli/blob/master/src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/commands.py#L19).
I don't really see this implementation as simpler than what's in the VM module as I now have to follow create_service_adapter to see what that does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see but as I am following the flow/structure from azure-cli-sql (as I have understood that flow as of now :)), I'd prefer to keep it same. What's your thought on that?
src/command_modules/azure-cli-monitor/azure/cli/command_modules/monitor/validators.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove if it's not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derekbekoe I was thinking that we'll not merge the PR in as it has sdk checked-in. So I was planning to wait until azure-mgmt-monitor releases and then removing sdks and the merging PR.
Let me know if there is way where we still allow merging PRs if sdks are checked-in? If so i'll definitely remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. We should NOT. No matter If azure-mgmt-monitor is released we should not include their source code in our code base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay didn't know you weren't planning on merging this right now.
I've created and added a Do Not Merge label to this PR then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have allowed services (KeyVault, IoT, etc.) to merge PRs with embedded clients. DocumentDB was about to earlier this week but the SDK was released so he went ahead and did the import changes.
If something holds up this PR from being merged, it really doesn't need to be the desire to avoid (temporarily) having the SDK code in the module.
|
|
|
|
|
|
|
|
|
|
|
|
|
@tjprescott & @derekbekoe I have just put some more comments on showing the documentation around commands for the reference. I haven't gone through all your comments but as I go I'll keep fixing them. This is just in case you want to see the commands itself 💯 I'll ping you guys once i fix all the review comments :) |
|
The help text for several of the list commands illustrates a major problem. First, the text from the SDK is crazily verbose, and we should replace it, but second, we should look at the equivalent Xplat CLI commands to see what convenience arguments they use to build up a filter. We should not simply expose the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a formality though it is not technically needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicate with the sql command module. Consider move it up to the azure-cli-core for reuse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should. I'd prefer it to do that refactoring once this PR completes so I don't confuse you guys more on actual PR. Does that seem good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer absolute path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to azure.cli.command_modules.monitor.validators
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these coming from SDK? shall we commit this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. I will not merge in until azure-mgmt-monitor is released. PR Azure/azure-sdk-for-python#974
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. We should NOT. No matter If azure-mgmt-monitor is released we should not include their source code in our code base.
Adding help docs for az monitor data plane commands Adding commands for management place of Azure monitor service Addressing code review comments
76e49eb to
4d8551e
Compare
|
@troydai @tjprescott @derekbekoe I've removed SDK code from the cli and took dependencies on I've also addressed general code review comments except:
Let me know what you guys think about new changes and how do we want to proceed. Thanks! |
…tions Implement filter for metrics-definitions list command
|
Implement filter for |
troydai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good.
|
@vishrutshah which commands remain that have filter building arguments in Xplat but not in this PR? |
|
Implement filter for @tjprescott With this PR merged in, we have following 2 commands from management plane left to be simplified for filter: |
Implement filter & select in activity-logs and tenant-activity-logs
* Adding scaffold command in autoscale-settings * Addressing review feedback
* Adding unittests for custom monitor commands * Better assertions
troydai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly questions
| timezone: 1970-01-01T00:00:00Z, 1970-01-01T00:00:00-0500 | ||
| :param str caller: The caller to look for when querying | ||
| :param str status: The status value to query (ex: Failed) | ||
| :param str max_events: The maximum number of records to be returned by the command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it str not int?
| caller, status) | ||
|
|
||
| if max_events: | ||
| max_events = int(max_events) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't argparser or validator guarantee it's integer?
| ''' | ||
| formatter = "eventTimestamp ge {} and eventTimestamp le {}" | ||
| odata_filters = _validate_time_range_and_add_defaults(start_time, end_time, | ||
| formatter=formatter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should time validating be done in validator?
|
|
||
|
|
||
| def _activity_logs_select_filter_builder(events=None): | ||
| '''Build up select filter string from events |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use triple double quote for docstring.
* Fix expand parameters bug in util to support any parameters * Adding --filters into activity-logs & tenant-activity-logs * Adding --ids support into alert-rules & autoscale-settings' show and delete commands * ignoring filter for alert-rules and autoscale-settings list
|
As we discussed Friday, we should close this PR until it is ready to be merged and then either reopen it or open a new one. |
|
Closing this PR and sending, new one for the review for partner team. |
|
New PR @ #2534 |
This enables support for #1954
Here is the complete set of available commands exposed from azure-mgmt-monitor and azure-monitor python sdk.
Here is the state of each Management command:
Here is the state of each Data Plane command:
Note: azure-mgmt-monitor is not released as python package yet, once released we'll take our dependency on locally checked-in sdk before merge.
@troydai @derekbekoe & @tjprescott when you get a chance, please have a first quick look. Thanks