Fix Monitor Tools Issues#2682
Conversation
MON-02: Use OrdinalIgnoreCase in BuildQuery limit detection to avoid
locale-sensitive false negatives (e.g. Turkish locale).
MON-05: Fix OData filter field name 'levels' -> 'level' in activity log
API call so level filtering is no longer silently ignored.
MON-06: Add structured try/catch to QueryWorkspace so exceptions are
logged and 404s surface as KeyNotFoundException, matching the
pattern used in QueryWorkspaceLogs.
MON-07: Remove ResourceGroup.AsRequired() from BaseMonitorCommand and
push it to only the three commands that actually use it
(TableListCommand, TableTypeListCommand, BaseMonitorHealthModelsCommand).
WorkspaceLogQueryCommand no longer requires --resource-group.
MON-08: Use OrdinalIgnoreCase (null-safe) in ListTables table-type
comparison so 'customlog' matches 'CustomLog'.
MON-10: Pass \={top} to the Activity Log API and break pagination
early once the requested count is reached, avoiding full
page traversal for small top values.
| description: | | ||
| Fix several correctness, reliability, and performance issues in the Monitor toolset: | ||
| - Activity log level filtering now works correctly. The OData filter used the wrong field name ('levels' instead of 'level'), causing the API to silently ignore the filter and return all severity levels regardless of what was requested. | ||
| - Workspace log queries no longer require --resource-group. The option was incorrectly enforced on all Monitor commands, including workspace queries that never use it. It is now only required on the commands that actually need it. |
There was a problem hiding this comment.
Are we sure the change here should be the workspace query command now takes resource group? Looking at its implementation it calls to list workspaces (which the tool there takes resource group) without using resource group.
Unless workspace name or ID is unique across subscriptions, then this is the correct change.
| catch (RequestFailedException ex) when (ex.Status == 404) | ||
| { | ||
| throw new KeyNotFoundException($"Workspace '{workspaceId}' not found.", ex); | ||
| } |
There was a problem hiding this comment.
Are we doing this remapping just to make the error message better? When we get RequestFailedException we use the Status to determine the statusCode JSON property in the response.
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error querying workspace '{WorkspaceId}'. Query: {Query}", workspaceId, query); | ||
| throw; |
There was a problem hiding this comment.
This isn't needed, the Command class calling this is already doing a try-catch-log, this will just create duplicate logs. Instead improve the log message in WorkspaceLogQueryCommand.
Also, we shouldn't log the query as it may contain PII.
2 participants
Fix several correctness, reliability, and performance issues in the Monitor toolset:
- Activity log level filtering now works correctly. The OData filter used the wrong field name ('levels' instead of 'level'), causing the API to silently ignore the filter and return all severity levels regardless of what was requested.
- Workspace log queries no longer require --resource-group. The option was incorrectly enforced on all Monitor commands, including workspace queries that never use it. It is now only required on the commands that actually need it.
- Table-type filtering is now case-insensitive. Passing 'customlog' instead of 'CustomLog' previously returned an empty list with no error. Filtering now matches regardless of casing.
- Query limit detection is now locale-safe. The 'limit' keyword check used a culture-sensitive comparison that could fail in Turkish locale, causing a duplicate limit clause and invalid KQL. This now uses ordinal comparison.
- QueryWorkspace now logs and classifies errors correctly. Exceptions previously propagated as bare stack traces; they are now caught, logged with context, and 404s are surfaced as a clear 'not found' error.
- Activity log pagination now stops as soon as the requested number of results is collected. Previously, all pages were fetched into memory before truncation, causing unnecessary ARM round-trips and potential timeouts on busy subscriptions.