Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Metrics Advisor] Add remaining docstrings #15792

Merged
6 commits merged into from
Oct 8, 2020
Merged

[Metrics Advisor] Add remaining docstrings #15792

6 commits merged into from
Oct 8, 2020

Conversation

christothes
Copy link
Member

No description provided.

@christothes christothes self-assigned this Oct 8, 2020
@christothes christothes changed the title Add remaining docstrings [Metrics Analyzer] Add remaining docstrings Oct 8, 2020
@christothes christothes changed the title [Metrics Analyzer] Add remaining docstrings [Metrics Advisor] Add remaining docstrings Oct 8, 2020
@christothes christothes added the Client This issue points to a problem in the data-plane of the library. label Oct 8, 2020
kinelski
kinelski reviewed Oct 8, 2020
View reviewed changes
Copy link
Member

@kinelski kinelski left a comment

Choose a reason for hiding this comment

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

Left some minor nit comments.

sdk/metricsadvisor/Azure.AI.MetricsAdvisor/src/Models/DataFeed/DataFeedAccessMode.cs Outdated Show resolved Hide resolved
sdk/metricsadvisor/Azure.AI.MetricsAdvisor/src/Models/DataFeed/DataFeedAutoRollupMethod.cs Outdated Show resolved Hide resolved
sdk/metricsadvisor/Azure.AI.MetricsAdvisor/src/Models/DataFeed/DataFeedIngestionSettings.cs Outdated Show resolved Hide resolved
...csadvisor/Azure.AI.MetricsAdvisor/src/Models/DataFeed/GetDataFeedIngestionStatusesOptions.cs Outdated Show resolved Hide resolved
sdk/metricsadvisor/Azure.AI.MetricsAdvisor/src/MetricsAdvisorAdministrationClient.cs Outdated Show resolved Hide resolved
sdk/metricsadvisor/Azure.AI.MetricsAdvisor/src/MetricsAdvisorClient.cs Outdated Show resolved Hide resolved
sdk/metricsadvisor/Azure.AI.MetricsAdvisor/src/MetricsAdvisorClient.cs Outdated
Comment on lines 1234 to 1248
/// <summary>
/// Gets data about the data points present in the specified set of time series.
/// <see cref="MetricAnomalyAlertConfiguration"/>.
/// </summary>
/// <param name="seriesKeys">The name of the dimension.</param>
/// <param name="detectionConfigurationId">The unique identifier of the <see cref="MetricAnomalyAlertConfiguration"/>.</param>
/// <param name="startTime">Filters the result. Only data points ingested after this point in time, in UTC, will be returned.</param>
/// <param name="endTime">Filters the result. Only data points ingested after this point in time, in UTC, will be returned.</param>
/// <param name="cancellationToken">A <see cref="CancellationToken"/> controlling the request lifetime.</param>
/// <returns>A collection containing the values the specified dimension assumed for anomalous data points. Items are unique.</returns>
/// <exception cref="ArgumentNullException"><paramref name="seriesKeys"/> or <paramref name="detectionConfigurationId"/> is null.</exception>
/// <exception cref="ArgumentException"><paramref name="seriesKeys"/> or <paramref name="detectionConfigurationId"/> is empty; or <paramref name="detectionConfigurationId"/> is not a valid GUID.</exception>
Copy link
Member

Choose a reason for hiding this comment

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

This description is very inaccurate. I think you copied it by accident from one the methods marked with TODODOCS (probably GetMetricEnrichedSeriesDataAsync).

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how I missed this, thanks.

sdk/metricsadvisor/Azure.AI.MetricsAdvisor/src/Models/AnomalyAlert/TopNGroupScope.cs Outdated Show resolved Hide resolved
...tricsadvisor/Azure.AI.MetricsAdvisor/src/Models/AnomalyDetection/ChangeThresholdCondition.cs Outdated Show resolved Hide resolved
...tricsadvisor/Azure.AI.MetricsAdvisor/src/Models/AnomalyDetection/ChangeThresholdCondition.cs Outdated Show resolved Hide resolved
maririos
maririos reviewed Oct 8, 2020
View reviewed changes
sdk/metricsadvisor/Azure.AI.MetricsAdvisor/src/MetricsAdvisorClient.cs
/// <summary>
/// Gets the automatic suggestions for likely root causes of an incident.
Copy link
Member

Choose a reason for hiding this comment

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

what does automatic suggestions means? what will be a "manual" suggestion?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess all suggestions are automatic :)

@maririos
Copy link
Member

maririos commented Oct 8, 2020

for all the TODODOCS that had questions about the service behavior, where the questions answered?

kinelski
kinelski approved these changes Oct 8, 2020
View reviewed changes
Copy link
Member

@kinelski kinelski left a comment

Choose a reason for hiding this comment

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

LGTM. Left some nit picky comments.

sdk/metricsadvisor/Azure.AI.MetricsAdvisor/src/MetricsAdvisorClient.cs Outdated Show resolved Hide resolved
sdk/metricsadvisor/Azure.AI.MetricsAdvisor/src/MetricsAdvisorClient.cs Outdated Show resolved Hide resolved
sdk/metricsadvisor/Azure.AI.MetricsAdvisor/src/MetricsAdvisorClient.cs Outdated Show resolved Hide resolved
sdk/metricsadvisor/Azure.AI.MetricsAdvisor/src/MetricsAdvisorClient.cs Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Oct 8, 2020

Hello @christothes!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

christothes and others added 6 commits October 8, 2020 17:16
@christothes
Add remaining docstrings
9bb9be2
@christothes @kinelski
Apply suggestions from code review
0a199d1 
Suggested edits

Co-authored-by: kinelski <camaiaor@microsoft.com>
@christothes
pr comments
650e2ae
@christothes
pr
b34e688
@christothes
pr comments
883d2f1
@christothes @maririos
Apply suggestions from code review
a5cd64f 
Co-authored-by: Mariana Rios Flores <mariari@microsoft.com>
@christothes christothes force-pushed the users/chriss/MA_remaining-docstrings branch from 97c6c71 to a5cd64f Compare October 8, 2020 22:17
@ghost ghost merged commit a5658b6 into Azure:master Oct 8, 2020
suhas92 pushed a commit to suhas92/azure-sdk-for-net that referenced this pull request Oct 12, 2020
@christothes @suhas92
[Metrics Advisor] Add remaining docstrings (Azure#15792)
d9a051a
annelo-msft pushed a commit to annelo-msft/azure-sdk-for-net that referenced this pull request Feb 17, 2021
@christothes @annelo-msft
[Metrics Advisor] Add remaining docstrings (Azure#15792)
a49be78
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kinelski kinelski kinelski approved these changes

@maririos maririos maririos approved these changes

Assignees

@christothes christothes

Labels
Client This issue points to a problem in the data-plane of the library. Cognitive - Metrics Advisor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@christothes @maririos @kinelski