Monitor query updates #17459

KarishmaGhiya · 2021-09-03T21:00:29Z

chradek · 2021-09-07T17:17:25Z

sdk/monitor/monitor-query/review/monitor-query.api.md

@@ -66,15 +65,19 @@ export interface LogsColumn {
 export type LogsColumnType = string;

 // @public
-export type LogsQueryBatchOptions = OperationOptions;
+export interface LogsQueryBatchOptions extends OperationOptions {
+    throwOnAnyFailure?: boolean;


Since we return a promise that can be rejected, I think rejectOnAnyFailure might be more accurate.

But remind me...so if I set this to true, then if any of the queries in the batch fails, the whole thing fails? The scenario you talked to me about before was disabling throwing an error on partial failures, so I'm curious about this.

Yes - this one will throw for both complete and partial failures. And the whole thing will fail in this case. But the users will get the data about all the responses. We wanted to follow similar approach across all JS sdks -

azure-sdk-for-js/sdk/search/search-documents/src/searchClient.ts

Line 493 in 8a7f4e2

if (options.throwOnAnyFailure && result._response.status === 207) {

Search-documents uses that flag - so thought of using the same approach.

Ah gotcha, if search-documents already uses that flag, then disregard my suggestion. Rather keep it consistent with throwOnAnyFailure then.

sdk/monitor/monitor-query/README.md

sdk/monitor/monitor-query/src/models/publicLogsModels.ts

sdk/monitor/monitor-query/README.md

maorleger · 2021-09-07T17:10:52Z

sdk/monitor/monitor-query/samples-dev/metricsQuery.ts

  while (!result.done) {
    console.log(` metricDefinitions - ${result.value.id}, ${result.value.name}`);
+    secondMetricName = result.value.name!;


Can we use for await(const result of iterator) instead?

Also...this has the potential to return lots of results right? Otherwise you'd exit the loop as soon as you got the 2nd metric. Can you collect all the names to pass to the query method?

yeah we can collect all the names and pass it to query method. Should I change the sample to that?

I am thinking of letting this be - and may be adding a comment that they can pass in all the names to the query method. What do you think? @chradek

sdk/monitor/monitor-query/samples/v1/javascript/logsQuery.js

sdk/monitor/monitor-query/src/internal/modelConverters.ts

sdk/monitor/monitor-query/review/monitor-query.api.md

sdk/monitor/monitor-query/src/internal/modelConverters.ts

maorleger · 2021-09-07T20:22:27Z

sdk/monitor/monitor-query/src/logsQueryClient.ts

-    return convertResponseForQueryBatch(flatResponse, rawResponse);
+    const result: LogsQueryBatchResult = convertResponseForQueryBatch(flatResponse, rawResponse);
+    if (options?.throwOnAnyFailure && result.batchResultStatus !== "AllSucceeded") {
+      throw result;


Same as above

maorleger · 2021-09-07T20:22:48Z

sdk/monitor/monitor-query/src/logsQueryClient.ts

+      result.logsQueryResultStatus = "Success";
+    }
+    if (options?.throwOnAnyFailure && result.logsQueryResultStatus !== "Success") {
+      throw result;


hmmm I'm not sure if throwing a result object is the right thing here - we should throw something that is an Error object

Yeah - we are just following what search documents is doing - in case if i throw an error - then the user won't get the data for queries that succeeded.

I looked at the guidelines on error handling and while it's not definitively stated that we should always use an ErrorLike I can think of a few reasons why we should be careful with this approach:

I expect an error to have a name and message property (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/message)

I am writing a UI app and I just put error.toString() in a banner - what would that look like? [object Object]?

I guess it's better if I use promise API but even then, catch expects rejected with a reason as the argument...

Not sure, I think it feels weird to throw something that is not an error. @bterlson or @chradek might convince me otherwise and maybe I have it all wrong in which case it'll be a good chance to learn what is right 😄

Edit to add: maybe I'm just not thinking about it right because it's a partial failure / partial success scenario...

I think I agree we should have a name, code, and message property (i.e. be error-like), otherwise error handling becomes awkward. Additional properties are fine if needed, though should be used sparingly since TS tooling doesn't help with them without the user explicitly casting.

It's unfortunate that search-documents isn't this way, probably in part because thrown errors don't show up in the API review 😕 I wonder what motivated the search-documents design, does anyone know?

We'd have to ask @xirzec but I wouldn't be surprised if it was an oversight. It wouldn't have shown up in the API review file and the PR that added it was pretty massive (#7482)

I do also think we should throw something error-like. I suspect users would expect message to be available at the very least.

I am not sure about the complete reasoning for search-documents, but for monitor-query and search-documents - one thing is true that we have multiple results being returned in that one response. For monitor query, in a single response we have multiple result objects and each of them has an error property that has the structure - similar to the guidelines - https://apiview.dev/Assemblies/Review/06691b2d79d543a58a95ad4d0ea22ed4#@azure/monitor-query!ErrorInfo:interface

We need to figure out what to do when user sets the "throwOnAnyError" flag -

Should we throw the first error?

Should we throw a list of all errors

In cases of partial errors (i.e, for responses that have both table property and error property), should we throw both table and error property ?

Other languages (.NET and Java) are throwing the errors on cases of complete errors and are throwing on partial errors if that flag is set.

This is what python is doing - https://gist.github.com/rakshith91/d08caf649feac0081703d4c1e5850326

sdk/monitor/monitor-query/test/internal/unit/modelConverters.unittest.spec.ts

Co-authored-by: Scott Addie <10702007+scottaddie@users.noreply.github.com>

sdk/monitor/monitor-query/samples-dev/metricsQuery.ts

sdk/monitor/monitor-query/samples/v1/javascript/metricsQuery.js

sdk/monitor/monitor-query/src/internal/modelConverters.ts

chradek · 2021-09-09T02:52:07Z

sdk/monitor/monitor-query/src/internal/modelConverters.ts

+    getMetricByName(metricName) {
+      return this.metrics.find((it) => it.name === metricName);
+    }


I think you could optimize this by defining the function getMetricByName outside of this function, and just assigned it to the MetricsQueryResult object.

Currently, you're creating a new function for every metric result. Changing how this is built would allow you to re-use a single function.

Rough example:

function getMetricByName(metricName: string) { return this.metrics.find((it) => it.name === metricName); } function createMetricsQueryResult(...params): MetricsQueryResult { return { ...params, getMetricByName }; }

sdk/monitor/monitor-query/src/internal/modelConverters.ts

chradek · 2021-09-09T02:53:55Z

sdk/monitor/monitor-query/src/internal/modelConverters.ts

+  } catch (e) {
+    return {
+      visualization: response.body?.render,
+      status: "Success",


An error occurred, but the status is Success?

I am updating it in the conditions later. I couldn't put a complex logic in this one line

chradek · 2021-09-09T02:55:25Z

sdk/monitor/monitor-query/src/logsQueryClient.ts

+    if (result.tables && result.error) {
+      result.status = "Partial";
+    } else if (result.error && !result.tables) {
+      result.status = "Failed";
+    } else {
+      result.status = "Success";
+    }


That's interesting. In a different part of your code, you have Failed as the fall-through case, but here it's success. Any reason for the difference?

No reason really. But now I am thinking if i should refine this - since the tables is never optional in the single query response.

if (!result.error) { // if there is no error field, it is success result.status = "Success"; } else { // result.tables is always present in single query response, even is there is error if (result.tables.length === 0) { result.status = "Failed"; } else { result.status = "Partial"; } } ```

How about this? So in the other batch query response - the tables can be undefined, but in single query response the "tables" field is always returned. So I think the above code snippet in the comment seems more appropriate for single query.

@chradek wanted follow up on this

Co-authored-by: chradek <51000525+chradek@users.noreply.github.com>

maorleger

🚀 a few nits but I'll leave up to you! We should make an issue for addressing the throw result before GA'ing this library

maorleger · 2021-09-09T18:37:28Z

sdk/monitor/monitor-query/src/models/publicLogsModels.ts

-    status?: number;
-    /** The list of tables, columns and rows. */
-    // (hoisted up from `LogQueryResult`)
+  results: {


Is this just LogsQueryResult[] or Array<LogsQueryResult>? Maybe we can reuse the type instead of duplicating if so

sdk/monitor/monitor-query/src/models/publicLogsModels.ts

sdk/monitor/monitor-query/src/models/publicMetricsModels.ts

…iya/azure-sdk-for-js into monitor-query-updates

sdk/monitor/monitor-query/CHANGELOG.md

sdk/monitor/monitor-query/README.md

Co-authored-by: Scott Addie <10702007+scottaddie@users.noreply.github.com>

…iya/azure-sdk-for-js into monitor-query-updates

KarishmaGhiya requested review from maorleger and richardpark-msft as code owners September 3, 2021 21:00

ghost added the Monitor Monitor, Monitor Ingestion, Monitor Query label Sep 3, 2021

KarishmaGhiya added 10 commits September 4, 2021 02:02

rename column to columnDescriptors and enums for error status

b2c7bad

sample for second metric

97492a4

update metrics query client

a00fe8c

result status

57216a4

update samples

772a1ca

getMetricByNames, flag for throwOnAnyFailure

a743827

refdocs and samples

4f74a91

regenerate swagger with innerError rename

206a91c

regenerate swagger with innerError rename

6b5ab13

update api review

26a3440

KarishmaGhiya force-pushed the monitor-query-updates branch from 8ef3e6a to 26a3440 Compare September 4, 2021 09:02

KarishmaGhiya requested review from chradek, xirzec and scottaddie and removed request for richardpark-msft September 4, 2021 09:03

chradek reviewed Sep 7, 2021

View reviewed changes

KarishmaGhiya added 6 commits September 7, 2021 11:57

fix lint errors

a427687

update metricsQuery sample

ff7cdfd

update KQL for multiple workspaces

c75643f

update readme

7b9020d

update readme

227bbb8

update readme

60df0c5

scottaddie reviewed Sep 7, 2021

View reviewed changes

scottaddie requested changes Sep 7, 2021

View reviewed changes

sdk/monitor/monitor-query/README.md Outdated Show resolved Hide resolved

sdk/monitor/monitor-query/README.md Outdated Show resolved Hide resolved

sdk/monitor/monitor-query/README.md Outdated Show resolved Hide resolved

sdk/monitor/monitor-query/README.md Outdated Show resolved Hide resolved

update readme

dbc6cd4

maorleger reviewed Sep 7, 2021

View reviewed changes

Update sdk/monitor/monitor-query/README.md

0941991

Co-authored-by: Scott Addie <10702007+scottaddie@users.noreply.github.com>

KarishmaGhiya added 5 commits September 8, 2021 15:27

update conversion

75c62d6

update readme

da9a5c1

added changelog

8a9ee1b

rename

265e265

readme

170dd4a

chradek reviewed Sep 9, 2021

View reviewed changes

Update sdk/monitor/monitor-query/src/internal/modelConverters.ts

f89cd47

Co-authored-by: chradek <51000525+chradek@users.noreply.github.com>

maorleger approved these changes Sep 9, 2021

View reviewed changes

This was referenced Sep 9, 2021

Optimize the function getMetricByName outside of this function, and just assigned it to the MetricsQueryResult object. #17544

Closed

Followups from PR #17459 #17545

Closed

KarishmaGhiya added 5 commits September 9, 2021 14:59

update Error types for batch and updated samples

d2ea6ce

Merge branch 'monitor-query-updates' of https://github.com/KarishmaGh…

51b3106

…iya/azure-sdk-for-js into monitor-query-updates

update models

6cdc3c0

update models

aa05c8a

update changelog

6adbbb8

KarishmaGhiya enabled auto-merge (squash) September 9, 2021 22:23

update api review

d762918

scottaddie reviewed Sep 9, 2021

View reviewed changes

sdk/monitor/monitor-query/CHANGELOG.md Outdated Show resolved Hide resolved

sdk/monitor/monitor-query/README.md Outdated Show resolved Hide resolved

sdk/monitor/monitor-query/README.md Outdated Show resolved Hide resolved

update reamde sample

6554230

KarishmaGhiya disabled auto-merge September 9, 2021 23:07

KarishmaGhiya and others added 5 commits September 9, 2021 16:08

Update sdk/monitor/monitor-query/CHANGELOG.md

41ba681

Co-authored-by: Scott Addie <10702007+scottaddie@users.noreply.github.com>

Update sdk/monitor/monitor-query/README.md

6479744

Co-authored-by: Scott Addie <10702007+scottaddie@users.noreply.github.com>

Update sdk/monitor/monitor-query/README.md

74baa6f

Co-authored-by: Scott Addie <10702007+scottaddie@users.noreply.github.com>

update logic for single query status compute

a5d0d52

Merge branch 'monitor-query-updates' of https://github.com/KarishmaGh…

e78334e

…iya/azure-sdk-for-js into monitor-query-updates

KarishmaGhiya enabled auto-merge (squash) September 9, 2021 23:43

KarishmaGhiya merged commit 9efb573 into Azure:main Sep 9, 2021

KarishmaGhiya mentioned this pull request Sep 15, 2021

Investigate Monitor Query tests timing out (14 failing tests) #17520

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Monitor query updates #17459

Monitor query updates #17459

KarishmaGhiya commented Sep 3, 2021 •

edited

Loading

chradek Sep 7, 2021

KarishmaGhiya Sep 7, 2021

chradek Sep 7, 2021

maorleger Sep 7, 2021

chradek Sep 7, 2021

KarishmaGhiya Sep 7, 2021

KarishmaGhiya Sep 8, 2021

maorleger Sep 7, 2021

maorleger Sep 7, 2021 •

edited

Loading

KarishmaGhiya Sep 7, 2021

maorleger Sep 7, 2021 •

edited

Loading

bterlson Sep 8, 2021

chradek Sep 8, 2021

KarishmaGhiya Sep 8, 2021

KarishmaGhiya Sep 8, 2021

chradek Sep 9, 2021

KarishmaGhiya Sep 9, 2021

chradek Sep 9, 2021

KarishmaGhiya Sep 9, 2021

chradek Sep 9, 2021

KarishmaGhiya Sep 9, 2021

KarishmaGhiya Sep 9, 2021

KarishmaGhiya Sep 9, 2021

KarishmaGhiya Sep 15, 2021

maorleger left a comment

maorleger Sep 9, 2021

KarishmaGhiya Sep 9, 2021

Monitor query updates #17459

Monitor query updates #17459

Conversation

KarishmaGhiya commented Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maorleger Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maorleger Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maorleger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarishmaGhiya commented Sep 3, 2021 •

edited

Loading

maorleger Sep 7, 2021 •

edited

Loading

maorleger Sep 7, 2021 •

edited

Loading