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

Api Consistency Review feedback #17873

Merged
merged 14 commits into from
Sep 29, 2021
Merged

Api Consistency Review feedback #17873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
8926fb8
update constants
KarishmaGhiya Sep 27, 2021
b6be064
update api review
KarishmaGhiya Sep 27, 2021
dc4b748
switch default end point
KarishmaGhiya Sep 27, 2021
878ed60
update acc to review
KarishmaGhiya Sep 27, 2021
8ed823b
add audience
KarishmaGhiya Sep 27, 2021
e0d43cf
remove values of audience
KarishmaGhiya Sep 27, 2021
bdf7b27
make audience just a string
KarishmaGhiya Sep 27, 2021
bb8aa98
update LogsErrorInfo object
KarishmaGhiya Sep 28, 2021
6a0971d
api review
KarishmaGhiya Sep 28, 2021
6afc0f7
fixed the listNamespace return list type
KarishmaGhiya Sep 28, 2021
ce89875
update API review
KarishmaGhiya Sep 28, 2021
b5fee3a
remove formatScope
KarishmaGhiya Sep 29, 2021
011e6a1
update refdocs
KarishmaGhiya Sep 29, 2021
d077ea0
updated API with refdocs
KarishmaGhiya Sep 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 31 additions & 52 deletions sdk/monitor/monitor-query/review/monitor-query.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,57 +11,33 @@ import { TokenCredential } from '@azure/core-auth';

// @public
export class AggregateBatchError extends Error {
constructor(errors: ErrorInfo[]);
// (undocumented)
constructor(errors: LogsErrorInfo[]);
errors: BatchError[];
}

// @public
export type AggregationType = "None" | "Average" | "Count" | "Minimum" | "Maximum" | "Total";

// @public
export class BatchError extends Error implements ErrorInfo {
constructor(errorInfo: ErrorInfo);
additionalProperties?: Record<string, unknown>;
export class BatchError extends Error implements LogsErrorInfo {
constructor(errorInfo: LogsErrorInfo);
code: string;
details?: ErrorDetail[];
innerError?: ErrorInfo;
message: string;
}

// @public
export const Durations: {
readonly sevenDays: "P7D";
readonly threeDays: "P3D";
readonly twoDays: "P2D";
readonly OneDay: "P1D";
readonly OneHour: "PT1H";
readonly FourHours: "PT4H";
readonly TwentyFourHours: "P1D";
readonly FourtyEightHours: "P2D";
readonly ThirtyMinutes: "PT30M";
readonly FiveMinutes: "PT5M";
readonly oneDay: "P1D";
readonly oneHour: "PT1H";
readonly fourHours: "PT4H";
readonly twentyFourHours: "P1D";
readonly fourtyEightHours: "P2D";
readonly thirtyMinutes: "PT30M";
readonly fiveMinutes: "PT5M";
};

// @public
export interface ErrorDetail {
additionalProperties?: Record<string, unknown>;
code: string;
message: string;
resources?: string[];
target?: string;
value?: string;
}

// @public
export interface ErrorInfo extends Error {
additionalProperties?: Record<string, unknown>;
code: string;
details?: ErrorDetail[];
innerError?: ErrorInfo;
message: string;
}

// @public
export interface ListMetricDefinitionsOptions extends OperationOptions {
metricNamespace?: string;
Expand All @@ -81,6 +57,11 @@ export interface LogsColumn {
// @public
export type LogsColumnType = string;

// @public
export interface LogsErrorInfo extends Error {
Copy link
Member

Choose a reason for hiding this comment

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

technically don't we only need code here? Error is defined as:

interface Error {
    name: string;
    message: string;
    stack?: string;
}

I don't feel too strongly about this as redeclaring the properties isn't hurting anything, but I wanted to point it out

Copy link
Member Author

Choose a reason for hiding this comment

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

Also is stack something that's defined internally? I am not assigning any values to stack though. Should we push the entire error object returned by the service into the stack?

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 removed the message property

Copy link
Contributor

Choose a reason for hiding this comment

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

When you instantiate an error, stack will be defined on it. So if you push BatchError onto your failed results, each one should have a stack property.

code: string;
}

// @public
export interface LogsQueryBatchOptions extends OperationOptions {
throwOnAnyFailure?: boolean;
Expand All @@ -90,7 +71,7 @@ export interface LogsQueryBatchOptions extends OperationOptions {
export interface LogsQueryBatchResult {
results: {
tables?: LogsTable[];
error?: ErrorInfo;
error?: LogsErrorInfo;
status?: LogsQueryResultStatus;
statistics?: Record<string, unknown>;
visualization?: Record<string, unknown>;
Expand All @@ -100,15 +81,13 @@ export interface LogsQueryBatchResult {
// @public
export class LogsQueryClient {
constructor(tokenCredential: TokenCredential, options?: LogsQueryClientOptions);
query(workspaceId: string, query: string, timespan: TimeInterval, options?: LogsQueryOptions): Promise<LogsQueryResult>;
queryBatch(batch: QueryBatch[], options?: LogsQueryBatchOptions): Promise<LogsQueryBatchResult>;
queryWorkspace(workspaceId: string, query: string, timespan: QueryTimeInterval, options?: LogsQueryOptions): Promise<LogsQueryResult>;
}

// @public
export interface LogsQueryClientOptions extends CommonClientOptions {
credentialOptions?: {
credentialScopes?: string | string[];
};
audience?: string;
endpoint?: string;
}

Expand All @@ -123,15 +102,15 @@ export interface LogsQueryOptions extends OperationOptions {

// @public
export interface LogsQueryResult {
error?: ErrorInfo;
error?: LogsErrorInfo;
statistics?: Record<string, unknown>;
status: LogsQueryResultStatus;
tables: LogsTable[];
visualization?: Record<string, unknown>;
}

// @public
export type LogsQueryResultStatus = "Partial" | "Success" | "Failed";
export type LogsQueryResultStatus = "PartialFailure" | "Success" | "Failure";

// @public
export interface LogsTable {
Expand Down Expand Up @@ -160,8 +139,8 @@ export interface Metric {

// @public
export interface MetricAvailability {
granularity?: string;
retention?: string;
timeGrain?: string;
}

// @public
Expand Down Expand Up @@ -195,16 +174,16 @@ export interface MetricNamespace {

// @public
export interface MetricsClientOptions extends CommonClientOptions {
audience?: string;
endpoint?: string;
}

// @public
export class MetricsQueryClient {
constructor(tokenCredential: TokenCredential, options?: MetricsClientOptions);
listMetricDefinitions(resourceUri: string, options?: ListMetricDefinitionsOptions): PagedAsyncIterableIterator<MetricDefinition>;
// Warning: (ae-forgotten-export) The symbol "MetricNamespace" needs to be exported by the entry point index.d.ts
listMetricNamespaces(resourceUri: string, options?: ListMetricNamespacesOptions): PagedAsyncIterableIterator<MetricNamespace_2>;
query(resourceUri: string, metricNames: string[], options?: MetricsQueryOptions): Promise<MetricsQueryResult>;
listMetricNamespaces(resourceUri: string, options?: ListMetricNamespacesOptions): PagedAsyncIterableIterator<MetricNamespace>;
queryResource(resourceUri: string, metricNames: string[], options?: MetricsQueryOptions): Promise<MetricsQueryResult>;
}

// @public
Expand All @@ -215,7 +194,7 @@ export interface MetricsQueryOptions extends OperationOptions {
metricNamespace?: string;
orderBy?: string;
resultType?: ResultType;
timespan?: TimeInterval;
timespan?: QueryTimeInterval;
top?: number;
}

Expand All @@ -227,7 +206,7 @@ export interface MetricsQueryResult {
metrics: Metric[];
namespace?: string;
resourceRegion?: string;
timespan: TimeInterval;
timespan: QueryTimeInterval;
}

// @public
Expand All @@ -253,15 +232,12 @@ export interface QueryBatch {
includeVisualization?: boolean;
query: string;
serverTimeoutInSeconds?: number;
timespan: TimeInterval;
timespan: QueryTimeInterval;
workspaceId: string;
}

// @public
export type ResultType = "Data" | "Metadata";

// @public
export type TimeInterval = {
export type QueryTimeInterval = {
startTime: Date;
endTime: Date;
} | {
Expand All @@ -274,6 +250,9 @@ export type TimeInterval = {
duration: string;
};

// @public
export type ResultType = "Data" | "Metadata";

// @public
export interface TimeSeriesElement {
data?: MetricValue[];
Expand Down
4 changes: 2 additions & 2 deletions sdk/monitor/monitor-query/samples-dev/logsQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,13 @@ export async function main() {
includeQueryStatistics: true
};

const result = await logsQueryClient.query(
const result = await logsQueryClient.queryWorkspace(
monitorWorkspaceId,
kustoQuery,
// The timespan is an ISO8601 formatted time (or interval). Some common aliases
// are available (like OneDay, OneHour, FoutyEightHours, etc..) but any properly formatted ISO8601
// value is valid.
{ duration: Durations.OneHour },
{ duration: Durations.oneHour },
queryLogsOptions
);

Expand Down
4 changes: 2 additions & 2 deletions sdk/monitor/monitor-query/samples-dev/logsQueryMultipleWorkspaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,13 @@ export async function main() {
additionalWorkspaces: [additionalWorkspaces1, additionalWorkspaces2]
};

const result = await logsQueryClient.query(
const result = await logsQueryClient.queryWorkspace(
monitorWorkspaceId,
kustoQuery,
// The timespan is an ISO8601 formatted time (or interval). Some common aliases
// are available (like durationOf1Day, durationOf1Hour, durationOf48Hours, etc..) but any properly formatted ISO8601
// value is valid.
{ duration: Durations.OneHour },
{ duration: Durations.oneHour },
queryLogsOptions
);

Expand Down
4 changes: 2 additions & 2 deletions sdk/monitor/monitor-query/samples-dev/metricsQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ export async function main() {

if (metricNames.length > 0) {
console.log(`Picking an example list of metrics to query: ${metricNames}`);
const metricsResponse = await metricsQueryClient.query(metricsResourceId, metricNames, {
const metricsResponse = await metricsQueryClient.queryResource(metricsResourceId, metricNames, {
granularity: "PT1M",
timespan: { duration: Durations.FiveMinutes }
timespan: { duration: Durations.fiveMinutes }
});

console.log(
Expand Down
16 changes: 6 additions & 10 deletions sdk/monitor/monitor-query/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export {
LogsTable,
LogsColumn,
LogsQueryResultStatus,
ErrorInfo,
LogsErrorInfo,
BatchError,
AggregateBatchError
} from "./models/publicLogsModels";
Expand All @@ -33,20 +33,20 @@ export {
MetricsQueryOptions,
MetricsQueryResult,
TimeSeriesElement,
MetricNamespace
MetricNamespace,
MetricAvailability
} from "./models/publicMetricsModels";

export { Durations } from "./models/constants";
export { TimeInterval } from "./models/timeInterval";
export { QueryTimeInterval } from "./models/timeInterval";
//
// LogsClient: generated exports
//

export {
// TODO: these are the generated model names. We probably want to run them
// through a manual review to make them consistent with style.
LogsColumnType,
ErrorDetail
LogsColumnType
} from "./generated/logquery/src";

//
Expand All @@ -60,9 +60,5 @@ export {
MetricUnit
} from "./generated/metrics/src";

export {
AggregationType,
MetricAvailability,
MetricClass
} from "./generated/metricsdefinitions/src";
export { AggregationType, MetricClass } from "./generated/metricsdefinitions/src";
export { NamespaceClassification } from "./generated/metricsnamespaces/src";
34 changes: 26 additions & 8 deletions sdk/monitor/monitor-query/src/internal/modelConverters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,15 @@ import {
Metric,
MetricDefinition,
TimeSeriesElement,
createMetricsQueryResult
createMetricsQueryResult,
MetricAvailability
} from "../models/publicMetricsModels";
import { FullOperationResponse } from "@azure/core-client";
import {
convertIntervalToTimeIntervalObject,
convertTimespanToInterval
} from "../timespanConversion";
import { ErrorInfo, LogsQueryResult } from "../models/publicLogsModels";
import { LogsErrorInfo, LogsQueryResult } from "../models/publicLogsModels";

/**
* @internal
Expand Down Expand Up @@ -137,11 +138,11 @@ export function convertResponseForQueryBatch(
for (let i = 0; i < resultsCount; i++) {
const result = newResponse.results[i];
if (result.error && result.tables) {
result.status = "Partial";
result.status = "PartialFailure";
} else if (result.tables) {
result.status = "Success";
} else {
result.status = "Failed";
result.status = "Failure";
}
}
(newResponse as any)["__fixApplied"] = fixApplied;
Expand Down Expand Up @@ -309,7 +310,7 @@ export function convertResponseForMetricsDefinitions(
generatedResponse: Array<GeneratedMetricDefinition>
): Array<MetricDefinition> {
const definitions: Array<MetricDefinition> = generatedResponse?.map((genDef) => {
const { name, dimensions, displayDescription, ...rest } = genDef;
const { name, dimensions, displayDescription, metricAvailabilities, ...rest } = genDef;

const response: MetricDefinition = {
...rest
Expand All @@ -322,6 +323,18 @@ export function convertResponseForMetricsDefinitions(
response.name = name.value;
}

const mappedMetricAvailabilities:
| Array<MetricAvailability>
| undefined = metricAvailabilities?.map((genMetricAvail) => {
return {
granularity: genMetricAvail.timeGrain,
retention: genMetricAvail.retention
};
});

if (mappedMetricAvailabilities) {
response.metricAvailabilities = mappedMetricAvailabilities;
}
const mappedDimensions = dimensions?.map((dim) => dim.value);

if (mappedDimensions) {
Expand Down Expand Up @@ -422,12 +435,17 @@ export function convertBatchQueryResponseHelper(
}
}

export function mapError(error?: GeneratedErrorInfo): ErrorInfo | undefined {
export function mapError(error?: GeneratedErrorInfo): LogsErrorInfo | undefined {
if (error) {
let innermostError = error;
while (innermostError.innerError) {
innermostError = innermostError.innerError;
}

return {
...error,
name: "Error",
innerError: mapError(error.innerError)
code: error.code,
message: `${error.message}. ${innermostError.message}`
};
}
return undefined;
Expand Down
Loading