Skip to content

[Monitor] Add Public Preview APIs of Metric Baseline #2049

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
Merged
sergey-shandar merged 4 commits into from
Dec 14, 2017
Merged

[Monitor] Add Public Preview APIs of Metric Baseline #2049

sergey-shandar merged 4 commits into from
Dec 14, 2017

Conversation

@asafst
Copy link
Contributor

@asafst asafst commented Nov 27, 2017 •
edited by azuresdkci
Loading

These APIs include:

  1. Getting the pre-calculated baseline of a metric
  2. Calculating the baseline of a metric given the metric values

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
  • If applicable, the PR references the bug/issue that it fixes.
  • Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

@asafst
[Monitor] Add Public Preview APIs of Metric Baseline
8ac999c 
These APIs include:
1. Getting the pre-calculated baseline of a metric
2. Calculating the baseline of a metric given the metric values
@sergey-shandar sergey-shandar added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Nov 27, 2017
@sergey-shandar
Copy link
Contributor

sergey-shandar commented Dec 6, 2017

@simongdavies could you have a look?

ravbhatnagar
ravbhatnagar reviewed Dec 7, 2017
View reviewed changes
specification/monitor/resource-manager/microsoft.insights/2017-11-01-preview/baseline_API.json
}
},
"paths": {
"/{resourceUri}/providers/microsoft.insights/baseline/{metricName}": {
Copy link
Contributor

@ravbhatnagar ravbhatnagar Dec 7, 2017

Choose a reason for hiding this comment

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

Description should state/show examples of resourceUri

Copy link
Contributor Author

@asafst asafst Dec 10, 2017

Choose a reason for hiding this comment

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

Added the description to the ResourceUriParameter parameter part.

specification/monitor/resource-manager/microsoft.insights/2017-11-01-preview/baseline_API.json
"BaselineResponse": {
"type": "object",
"properties": {
"id": {
Copy link
Contributor

@ravbhatnagar ravbhatnagar Dec 7, 2017

Choose a reason for hiding this comment

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

id, name and type properties should be marked as readonly: true

Copy link
Contributor Author

@asafst asafst Dec 10, 2017

Choose a reason for hiding this comment

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

Added readonly:true

specification/monitor/resource-manager/microsoft.insights/2017-11-01-preview/baseline_API.json
"$ref": "#/definitions/BaselineProperties",
"description": "the properties of the baseline."
},
"timestamps": {
Copy link
Contributor

@ravbhatnagar ravbhatnagar Dec 7, 2017

Choose a reason for hiding this comment

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

timestamps, baseline, metadata should be under properties envelope.

Copy link
Contributor Author

@asafst asafst Dec 10, 2017

Choose a reason for hiding this comment

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

Moved them to properties

specification/monitor/resource-manager/microsoft.insights/2017-11-01-preview/baseline_API.json
"Baseline": {
"type": "object",
"properties": {
"sensitivity": {
Copy link
Contributor

@ravbhatnagar ravbhatnagar Dec 7, 2017

Choose a reason for hiding this comment

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

enum?

Copy link
Contributor Author

@asafst asafst Dec 10, 2017

Choose a reason for hiding this comment

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

You're right, changed to enum

specification/monitor/resource-manager/microsoft.insights/2017-11-01-preview/baseline_API.json Outdated
},
"BaselineProperties": {
"properties": {
"timespan": {
Copy link
Contributor

@ravbhatnagar ravbhatnagar Dec 7, 2017

Choose a reason for hiding this comment

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

date-time format?

Copy link
Contributor Author

@asafst asafst Dec 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It should be an ISO 8601 time interval (see: https://en.wikipedia.org/wiki/ISO_8601#Time_intervals). The format is "{date-time}/{date-time}". So string is probably more appropriate here.
I tried to be consistent with the definitions and types in the metrics_API.

@azuresdkciprbot
Copy link

azuresdkciprbot commented Dec 10, 2017

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/monitor/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 1
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

azuresdkciprbot commented Dec 11, 2017

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/monitor/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 1
After the PR: Warning(s): 0 Error(s): 1

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@ravbhatnagar ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Dec 14, 2017
@ravbhatnagar
Copy link
Contributor

ravbhatnagar commented Dec 14, 2017

Looks good.

@sergey-shandar sergey-shandar merged commit e6af07c into Azure:current Dec 14, 2017
@AutorestCI
Copy link

AutorestCI commented Dec 14, 2017

No modification for AutorestCI/azure-sdk-for-ruby

@AutorestCI AutorestCI mentioned this pull request Dec 14, 2017
Merged
@AutorestCI
Copy link

AutorestCI commented Dec 14, 2017

Build Status Azure/azure-sdk-for-python#1710

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@ravbhatnagar ravbhatnagar ravbhatnagar left review comments

@sergey-shandar sergey-shandar sergey-shandar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sergey-shandar sergey-shandar

Labels

ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@asafst @sergey-shandar @azuresdkciprbot @ravbhatnagar @AutorestCI