Skip to content

Add options overload API's to Metrics Advisor Clients#3

Merged
anuchandy merged 3 commits into
anuchandy:new-api-impl-and-cleanupfrom
samvaity:add-list-alert-overload
May 28, 2021
Merged

Add options overload API's to Metrics Advisor Clients#3
anuchandy merged 3 commits into
anuchandy:new-api-impl-and-cleanupfrom
samvaity:add-list-alert-overload

Conversation

@samvaity

Copy link
Copy Markdown
Collaborator

@samvaity samvaity requested a review from anuchandy May 27, 2021 21:06
@samvaity samvaity self-assigned this May 27, 2021

@anuchandy anuchandy left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

LGTM, some minor comments, also be sure to run (just to catch any style validations)

mvn clean install -f sdk/metricsadvisor/pom.xml --batch-mode -Dmaven.wagon.http.pool=false --settings eng/settings.xml -DskipTests -Dinclude-template  -Dgpg.skip -Dmaven.javadoc.skip=false -Dcheckstyle.skip=false -Dspotbugs.skip=false -Drevapi.skip=true

public PagedFlux<MetricSeriesDefinition> listMetricSeriesDefinitions(
String metricId,
OffsetDateTime activeSince) {
return listMetricSeriesDefinitions(metricId, activeSince);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

this method call itself

@ServiceMethod(returns = ReturnType.COLLECTION)
public PagedFlux<AnomalyDetectionConfiguration> listMetricAnomalyDetectionConfigs(
String metricId) {
return listMetricAnomalyDetectionConfigs(metricId);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

this method seems recusring

String alertId,
ListIncidentsAlertedOptions options) {
return listIncidents(alertConfigurationId, alertId, options, Context.NONE);
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

we need below overload

public PagedIterable<AnomalyIncident> listIncidents(String alertConfigurationId, 
    String alertId);

String detectionConfigurationId,
String dimensionName,
OffsetDateTime startTime, OffsetDateTime endTime) {
return listAnomalyDimensionValues(detectionConfigurationId, dimensionName, startTime, endTime);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

this is infinite recursion.

@anuchandy anuchandy merged commit a3f4c7f into anuchandy:new-api-impl-and-cleanup May 28, 2021
@samvaity samvaity deleted the add-list-alert-overload branch June 22, 2021 20:40
anuchandy pushed a commit that referenced this pull request Apr 3, 2023
* health insights sdk for java

* rename country to countryOrRegion

* update product name to azure-health-insights

* update cspell with missing words

* update root pom.xml and version_client.txt

* update dependency versions

* update dependency versions (#2)

* skip jacoco

* fix spell checks

* Asaflevi/feature/healthinsights sdk for java (#3)

* update dependency versions

* skip jacoco

* fix spell checks

* skip jacoco

* linting

* remove redundant plugins

* remove test.yaml files

* remove redundant package.json file

* fix sampes and readmes

* spell

* fix linting

* fix sample repository reference

* remove duplicate dependency (mockito)

* alignment (readme)

* fix readme title

* setPlaybackSyncPollerPollInterval

* update serviceversion class (emitter 0.5.1)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants