Feat: support resource-specific tables for diagnostics #95
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces significant enhancements to Azure Kubernetes Service (AKS) control plane log querying by adding support for resource-specific diagnostic tables alongside the existing Azure Diagnostics tables. The changes improve the flexibility and accuracy of KQL query generation through a comprehensive refactoring of the query builder architecture.
Key changes include:
- Implementation of a new diagnostic setting finder that locates specific log categories and determines table mode
- Complete refactoring of KQL query generation from a single function to a modular class-based builder
- Addition of comprehensive mappings for resource-specific tables and log level handling across both table modes
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/components/monitor/diagnostics/workspace.go |
Added FindDiagnosticSettingForCategory function to locate diagnostic settings with specific log categories and determine resource-specific table usage |
internal/components/monitor/diagnostics/kql.go |
Complete refactoring from BuildSafeKQLQuery function to KQLQueryBuilder class with modular query construction supporting both Azure Diagnostics and resource-specific tables |
internal/components/monitor/diagnostics/handlers.go |
Updated HandleControlPlaneLogs to use new diagnostic setting finder and pass resource-specific flag to query builder |
internal/components/monitor/diagnostics/kql_test.go |
Extensive test coverage additions for resource-specific table functionality, log level mappings, and case sensitivity handling |
internal/components/monitor/diagnostics/workspace_test.go |
Added TestFindDiagnosticSettingForCategory test for diagnostic setting finder validation |
Comments suppressed due to low confidence (1)
internal/components/monitor/diagnostics/workspace_test.go:193
- This test only validates that an error occurs but doesn't verify the specific error handling logic or edge cases of FindDiagnosticSettingForCategory. Consider adding mock data or more specific test scenarios to verify the function's parsing logic, workspace ID extraction, and resource-specific flag determination.
expectError: true, // Will fail during Azure CLI execution in test environment
| clusterResourceID string // The resource ID of the cluster being queried. | ||
| isResourceSpecific bool // Indicates whether the query targets a resource-specific table. | ||
| actualTableMode TableMode // Specifies the mode of the table being queried (e.g., AzureDiagnosticsMode or ResourceSpecificMode). | ||
| // Note: `isResourceSpecific` and `actualTableMode` are related. If `isResourceSpecific` is true, |
There was a problem hiding this comment.
The relationship between isResourceSpecific and actualTableMode creates redundancy and potential inconsistency. Consider consolidating these into a single field or ensuring they are always synchronized through a setter method to prevent mismatched states.
Copilot uses AI. Check for mistakes.
| settingName = name | ||
| } | ||
|
|
||
| // Debug log which setting and workspace is being used |
There was a problem hiding this comment.
Using the standard library 'log' package for debug logging in production code can cause performance issues and lacks structured logging capabilities. Consider using a proper logging framework or making this conditional based on debug configuration.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
we don't have structured logs now.
This pull request introduces significant updates to enhance the handling of diagnostic settings and KQL query generation for Azure Kubernetes Service (AKS) control plane logs. The changes improve support for resource-specific tables, add flexibility in log category handling, and refactor the KQL query builder for better maintainability. Key updates include the addition of a diagnostic setting finder, a revamped query builder, and new test cases.
Enhancements to diagnostic settings handling:
FindDiagnosticSettingForCategoryfunction to locate diagnostic settings with the specified log category enabled, returning the workspace resource ID and whether resource-specific tables are used. Debug logging was added for better traceability. (internal/components/monitor/diagnostics/workspace.go, internal/components/monitor/diagnostics/workspace.goR103-R181)Refactoring of KQL query generation:
BuildSafeKQLQueryfunction with a newKQLQueryBuilderclass, which modularizes query construction into distinct steps such as table determination, log level filtering, and field projection. This refactor supports both Azure Diagnostics and resource-specific tables. (internal/components/monitor/diagnostics/kql.go, internal/components/monitor/diagnostics/kql.goL9-R196)internal/components/monitor/diagnostics/kql.go, internal/components/monitor/diagnostics/kql.goL9-R196)Updates to log category and table handling:
internal/components/monitor/diagnostics/kql.go, internal/components/monitor/diagnostics/kql.goL9-R196)Integration and use of new functionalities:
HandleControlPlaneLogsto use the newFindDiagnosticSettingForCategoryfunction and pass theisResourceSpecificflag to the KQL query builder, ensuring proper table selection. (internal/components/monitor/diagnostics/handlers.go, internal/components/monitor/diagnostics/handlers.goL72-R89)New test cases:
TestFindDiagnosticSettingForCategory, to validate the behavior of the diagnostic setting finder under various scenarios, including valid and invalid log categories. (internal/components/monitor/diagnostics/workspace_test.go, internal/components/monitor/diagnostics/workspace_test.goR176-R223)