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.

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 align the request bodies #16760

Merged
merged 12 commits into from
Aug 5, 2021

Conversation

KarishmaGhiya
Copy link
Member

@KarishmaGhiya KarishmaGhiya commented Aug 4, 2021

  • add additional workspaces parameter, moved it to options
  • Moved other optional params to options
  • rename workspace to workspaceId

Update:

  • This PR doesn't fix the tests.
  • (Tried to increase timeout. Figured this fix will not work for CI. )
    -Change the tests to last 2 days to combat slower ingestion times.

@ghost ghost added the Monitor Monitor, Monitor Ingestion, Monitor Query label Aug 4, 2021
@KarishmaGhiya KarishmaGhiya changed the title Monitor query align the request bodies Monitor query align the request bodies and fix the tests Aug 4, 2021
Copy link
Member

@maorleger maorleger left a comment

Choose a reason for hiding this comment

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

Glad we're fixing the tests! But I think at the very least additionalWorkspaces should be in the QueryLogsOptions bag, and I recommend renaming additionalWorkspaces to additionalWorkspaceIds

Of course, any of our architects can veto my change request here 😄

@@ -13,15 +13,12 @@ export type AggregationType = "None" | "Average" | "Count" | "Minimum" | "Maximu

// @public
export interface BatchQuery {
azureResourceIds?: string[];
additionalWorkspaces?: string[];
Copy link
Member

Choose a reason for hiding this comment

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

additionalWorkspaceIds maybe? Unless this is how all languages called it

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this is across all languages

@@ -83,7 +80,7 @@ export type LogsColumnType = string;
// @public
export class LogsQueryClient {
constructor(tokenCredential: TokenCredential, options?: LogsQueryClientOptions);
queryLogs(workspaceId: string, query: string, timespan: string, options?: QueryLogsOptions): Promise<QueryLogsResult>;
queryLogs(workspaceId: string, query: string, timespan: string, additionalWorkspaces?: string[], options?: QueryLogsOptions): Promise<QueryLogsResult>;
Copy link
Member

Choose a reason for hiding this comment

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

I think additionalWorkspaces should be in the options bag - at least that's how I'm used to seeing additional optional params

@@ -89,7 +91,8 @@ export class LogsQueryClient {
workspaceId,
{
query,
timespan
timespan,
workspaces: additionalWorkspaces?.map((x) => x)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need this map here right?

Copy link
Member Author

Choose a reason for hiding this comment

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

will it just copy it completely?

Copy link
Member

@maorleger maorleger left a comment

Choose a reason for hiding this comment

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

Just needs a changelog entry

@KarishmaGhiya KarishmaGhiya changed the title Monitor query align the request bodies and fix the tests Monitor query align the request bodies Aug 5, 2021
@KarishmaGhiya KarishmaGhiya linked an issue Aug 5, 2021 that may be closed by this pull request
@KarishmaGhiya KarishmaGhiya merged commit f85d382 into Azure:main Aug 5, 2021
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Nov 17, 2021
[Hub Generated] Review request for Microsoft.BotService to add version preview/2021-05-01-preview (Azure#16760)

* rename

* change to enum

* fix bug

* fix example
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Monitor Monitor, Monitor Ingestion, Monitor Query
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[monitor-query] Unclear parameters (workspaces and resourceUri)
2 participants