feat: refactor for simplifying adding a new default scrape config #1189
Conversation
…or-new-default-config
…or-new-default-config
…or-new-default-config
…or-new-default-config
…or-new-default-config
There was a problem hiding this comment.
Pull Request Overview
This PR aims to simplify the process of adding new default scrape configurations by consolidating scattered configuration definitions and logic into a centralized data structure. The refactoring centralizes job definitions, default values, and configuration logic into a single location.
Key changes include:
- Centralized all default scrape job configurations into a shared data structure
- Simplified configuration parsing by using the centralized definitions
- Refactored error handling to be more lenient (log and continue vs fail)
Reviewed Changes
Copilot reviewed 63 out of 69 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
otelcollector/shared/definitions.go |
New file containing centralized default scrape job configurations and constants |
otelcollector/shared/helpers.go |
Modified error handling in file parsing to log errors and continue |
otelcollector/shared/go.mod |
Updated dependencies and added new required packages |
otelcollector/shared/configmap/mp/tomlparser-*.go |
Refactored configuration parsing to use centralized definitions |
| Various test files | Updated test files and removed obsolete test files |
Comments suppressed due to low confidence (2)
otelcollector/shared/configmap/mp/tomlparser-opentelemetry-metrics.go:14
- The function name 'populateOpentelemetryMetricsSettingValuesFromConfigMap' is inconsistent with the function being called. The original code calls 'populateSettingValuesFromConfigMap' but the new code calls a different function name.
enabled := populateOpentelemetryMetricsSettingValuesFromConfigMap(metricsConfigBySection)
otelcollector/shared/configmap/mp/tomlparser-opentelemetry-metrics.go:41
- [nitpick] The error message format is improved but uses inconsistent variable reference. The original code referenced 'enabled' but now references 'err.Error()' which provides better context.
fmt.Printf("Invalid value for opentelemetry-metrics enabled: %s, defaulting to %v\n", err.Error(), enabled)
otelcollector/shared/helpers.go
Outdated
| file, err := os.Open(filePath) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to open file %s: %w", filePath, err) | ||
| fmt.Printf("failed to open file %s: %v\n", filePath, err) |
There was a problem hiding this comment.
The error message format is inconsistent with the original error handling pattern. Consider using a more descriptive error message that explains the impact of the failure.
| fmt.Printf("failed to open file %s: %v\n", filePath, err) | |
| log.Printf("error: failed to open file %s: %v. Skipping this file.", filePath, err) |
| return err | ||
| } | ||
| return nil | ||
| return os.WriteFile(filePath, out, 0644) |
There was a problem hiding this comment.
The file permissions are hardcoded as 0644. Consider using a constant or fs.FileMode for better maintainability and consistency.
| return os.WriteFile(filePath, out, 0644) | |
| return os.WriteFile(filePath, out, defaultFilePermissions) |
| job.KeepListRegex = job.CustomerKeepListRegex + job.KeepListRegex | ||
| } | ||
|
|
||
| fmt.Printf("Parsed config map for default-targets-metrics-keep-list successfully\n") |
There was a problem hiding this comment.
[nitpick] The success message is not consistent with error handling patterns in the codebase. Consider using a more structured logging approach or removing this debug message.
|
This PR is stale because it has been open 7 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
…or-new-default-config
|
This PR is stale because it has been open 7 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR is stale because it has been open 7 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
…or-new-default-config
…or-new-default-config
…or-new-default-config
https://dev.azure.com/msazure/InfrastructureInsights/_wiki/wikis/InfrastructureInsights.wiki/725465/SinglePlaceToAddNewScrapeJob