Skip to content

Update docs to indicate [Get/Disable/Enable]-AzHDInsightMonitoring cmdlets are 'Classic' #15533

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 4 commits into from
Jul 29, 2021

Conversation

proazr
Copy link
Contributor

@proazr proazr commented Jul 25, 2021

Description

This PR includes updates to the synopsis & descriptions for the Enable-AzHDInsightMonitoring, Disable-AzHDInsightMonitoring & Get-AzHDInsightMonitoring cmdlets to indicate that they are for managing the Classic Azure Monitor logs integration on an HDInsight cluster.

Following are a few reasons why this is necessary:

  1. At present, there are two sets of cmdlets for managing the integration of Azure Monitor Logs w/ an HDInsight cluster, namely:

    1. Enable-AzHDInsightMonitoring, Disable-AzHDInsightMonitoring & Get-AzHDInsightMonitoring
    2. Enable-AzHDInsightAzureMonitor, Disable-AzHDInsightAzureMonitor & Get-AzHDInsightAzureMonitor

    However, the help docs for both sets of cmdlets don't mention how they are different, i.e., the first set of cmdlets integrate the cluster w/ Azure Monitor Logs in the "classic" way, while the second set of cmdlets being the newer ones. This causes confusion and would require further researching to identify how they differ (happened to me).

    On the other hand, the docs for the Azure CLI HDInsight commands do highlight this difference - https://docs.microsoft.com/en-us/cli/azure/hdinsight/monitor?view=azure-cli-latest

  2. Here is an example on how the docs for Enable-AzHDInsightMonitoring & Enable-AzHDInsightAzureMonitor appear to be mostly be the same:

    image

    image

  3. The portal currently indicates the Log Analytics integration for HDInsight as "Classic" under Monitoring > Monitor Integration. It would help to indicate the same in the help docs for the respective set of cmdlets.

    image

  4. The classic integration of Azure Monitor Logs is soon to be deprecated and the recommendation is for customers to migrate to the newer integration as documented here - https://docs.microsoft.com/en-us/azure/hdinsight/log-analytics-migration. Updating the help docs to indicate that the first set of cmdlets are "classic" will help make users aware of this distinction.

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

proazr added 3 commits July 25, 2021 01:29
Updated synopsis & description to differentiate from the newer Disable-AzHDInsightAzureMonitor cmdlet.
Updated synopsis & description to differentiate from the newer Enable-AzHDInsightAzureMonitor cmdlet.
Updated synopsis & description to differentiate from the newer Get-AzHDInsightAzureMonitor cmdlet.
@ghost ghost added the customer-reported label Jul 25, 2021
@ghost
Copy link

ghost commented Jul 25, 2021

Thank you for your contribution proazr! We will review the pull request and get back to you soon.

@ghost
Copy link

ghost commented Jul 25, 2021

CLA assistant check
All CLA requirements met.

@isra-fel
Copy link
Member

@proazr thanks for your contribution! Let me route this to the correct team...

Hi @idear1203 @aim-for-better would you be able to help review this? Thanks!

@isra-fel isra-fel self-assigned this Jul 26, 2021
@proazr proazr changed the title Update docs to indicate [Get/Delete/Enable]-AzHDInsightMonitoring cmdlets are 'Classic' Update docs to indicate [Get/Disable/Enable]-AzHDInsightMonitoring cmdlets are 'Classic' Jul 26, 2021
@aim-for-better
Copy link
Member

Hi @isra-fel is changing the description a breaking change or not?

@isra-fel
Copy link
Member

Not a breaking change. It's fine.

Copy link
Member

@aim-for-better aim-for-better left a comment

Choose a reason for hiding this comment

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

@aim-for-better
Copy link
Member

Not a breaking change. It's fine.

Thanks

Updated synopsis for [Get/Disable/Enable]-AzHDInsightMonitoring cmdlets.
@proazr
Copy link
Contributor Author

proazr commented Jul 28, 2021

Copy link
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

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

Looking good. Let me merged the PR. Thank you both :)

@isra-fel isra-fel merged commit db2c910 into Azure:main Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants