[Monitor Query] move to tracing 1.0.0 #21260
Conversation
ghost
added
the
|
API change check for API changes have been detected in API changes - export interface ListMetricNamespacesOptions {
+ export interface ListMetricNamespacesOptions extends OperationOptions { |
| namespace: "Microsoft.Monitor", | ||
| packagePrefix: "Azure.Monitor.Query", | ||
| packageName: "@azure/monitor-query", |
There was a problem hiding this comment.
We can add the package version here is as well
packageVersion: SDK_VERSION // from constants.ts
| // maxPageSize: 2, | ||
| // continuationToken: null | ||
| // }), | ||
| // client.listMetricNames("resourceUri") |
There was a problem hiding this comment.
Unsure about the accurate way of testing the list methods with the supportsTracing . I put the error in the comment above cc: @maorleger @xirzec
There was a problem hiding this comment.
I think if you call next it should work. byPage itself is not an async function and doesn't make any calls. next starts fetching data
| @@ -123,10 +130,10 @@ export class MetricsQueryClient { | |||
| resourceUri: string, | |||
| options: ListMetricDefinitionsOptions = {} | |||
| ): AsyncIterableIterator<Array<MetricDefinition>> { | |||
| const segmentResponse = await this._definitionsClient.metricDefinitions.list( | |||
| const segmentResponse = await tracingClient.withSpan("MetricsQueryClient.listSegmentOfMetricDefinitions",options, async (updatedOptions) => await this._definitionsClient.metricDefinitions.list( | |||
There was a problem hiding this comment.
I'm surprised prettier didn't format this? Or maybe it did and that's why build is failing I haven't checked...
There was a problem hiding this comment.
fixed the formatting
| options | ||
| ), | ||
| client.queryBatch([ | ||
| { |
There was a problem hiding this comment.
Nit: since we ignore any and all errors anyway, maybe it makes sense to just make this the smallest possible batch that sends a request. For example does client.queryBatch([]) send a request to the server? Having all of this in here adds noise otherwise
There was a problem hiding this comment.
I reduced it to one query.
|
/azp run js - monitor - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
|
I think we might have to merge master to get changes from #21967 @mpodwysocki can you review this PR when you get a chance? |
KarishmaGhiya
force-pushed
the
tracing-monitor-query
branch
from
83f7446
to
38815f8
Compare
…t supported in browser
|
No pipelines are associated with this pull request. |
Fixes #21245
Packages impacted by this PR
@azure/monitor-query
Issues associated with this PR
Describe the problem that is addressed by this PR
Upgrading tracing to 1.0.0 for monitor query and setting up tracing for monitor query APIs.
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
Are there test cases added in this PR? (If not, why?)
Provide a list of related PRs (if any)
Command used to generate this PR:**(Applicable only to SDK release request PRs)
Checklists