-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Monitor query updates #17459
Monitor query updates #17459
Changes from 10 commits
b2c7bad
97492a4
a00fe8c
57216a4
772a1ca
a743827
4f74a91
206a91c
6b5ab13
26a3440
a427687
ff7cdfd
c75643f
7b9020d
227bbb8
60df0c5
dbc6cd4
0941991
31987fd
a929e08
3a54e4d
cbd309b
edf39ca
b924d3d
f64a7fc
8636437
5a21ffb
5e6d08b
7038eb4
fa238a8
41f82df
2ff1efb
5ac02ad
a17c343
6748d77
3bda4b4
5c930f2
fed387c
a0cec0e
9e6f633
75c62d6
da9a5c1
8a9ee1b
265e265
170dd4a
f89cd47
d2ea6ce
51b3106
6cdc3c0
aa05c8a
6adbbb8
d762918
6554230
41ba681
6479744
74baa6f
a5d0d52
e78334e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,26 +24,33 @@ export async function main() { | |
const iterator = metricsQueryClient.listMetricDefinitions(metricsResourceId); | ||
let result = await iterator.next(); | ||
const firstMetric: MetricDefinition = result.value; | ||
|
||
let secondMetricName: string; | ||
while (!result.done) { | ||
console.log(` metricDefinitions - ${result.value.id}, ${result.value.name}`); | ||
secondMetricName = result.value.name!; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah we can collect all the names and pass it to query method. Should I change the sample to that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
result = await iterator.next(); | ||
} | ||
console.log(`First Metric Definition = ${firstMetric.name}`); | ||
|
||
console.log(`Picking an example metric to query: ${firstMetric.name!}`); | ||
|
||
const metricsResponse = await metricsQueryClient.query(metricsResourceId, [firstMetric.name!], { | ||
granularity: "PT1M", | ||
timespan: { duration: Durations.FiveMinutes } | ||
}); | ||
const metricsResponse = await metricsQueryClient.query( | ||
metricsResourceId, | ||
[firstMetric.name!, secondMetricName!], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bcz of swagger model - Metric Definitions and Metric Namespaces can return empty objects as specified in the swagger by the service. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I get that the swagger model models the name as optional, but my question is, does the service actually treat name as optional or is it a mistake in the model? We're using the non-null assertion in our sample, which either means we believe it is actually always defined - so why make it optional - or we're not showing the customer how to handle a legitimate edge case. If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we pass undefined to query the service will return an error - so we shouldn't do that. I updated the sample to do the verification of both metric names being valid |
||
{ | ||
granularity: "PT1M", | ||
timespan: { duration: Durations.FiveMinutes } | ||
} | ||
); | ||
|
||
console.log( | ||
`Query cost: ${metricsResponse.cost}, interval: ${metricsResponse.granularity}, time span: ${metricsResponse.timespan}` | ||
); | ||
|
||
const metrics: Metric[] = metricsResponse.metrics; | ||
console.log(`Metrics:`, JSON.stringify(metrics, undefined, 2)); | ||
const metric = metricsResponse.getMetricByName(firstMetric.name!); | ||
console.log(`Selected Metric: ${firstMetric.name}`, JSON.stringify(metric, undefined, 2)); | ||
} | ||
|
||
main().catch((err) => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha, if search-documents already uses that flag, then disregard my suggestion. Rather keep it consistent with
throwOnAnyFailure
then.