-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
chore: address some gaps in k8s monitoring #6653
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to c795f58 in 1 minute and 45 seconds
More details
- Looked at
782
lines of code in16
files - Skipped
0
files when reviewing. - Skipped posting
11
drafted comments based on config settings.
1. pkg/query-service/app/metrics/query_builder.go:12
- Draft comment:
TheAddMetricValueFilter
function has repetitive code for different numeric types. Consider using a type switch with a single case for all numeric types to reduce redundancy. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a new filter__value
to handle non-aggregatable metrics by encoding state in labels. This is a good approach to make metrics aggregatable. However, the implementation ofAddMetricValueFilter
function inpkg/query-service/app/metrics/query_builder.go
can be optimized. The function currently has repetitive code for different numeric types, which can be simplified.
2. pkg/query-service/app/inframetrics/pods_query.go:54
- Draft comment:
TheSpaceAggregation
for utilization metrics should beSpaceAggregationAvg
instead ofSpaceAggregationSum
. This change is necessary to correctly aggregate percentage values. Please ensure this change is applied consistently across all relevant queries. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. pkg/query-service/app/metrics/v3/query_builder.go:336
- Draft comment:
TheAddMetricValueFilter
function has repetitive code for different numeric types. Consider using a type switch with a single case for all numeric types to reduce redundancy. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a new filter__value
to handle non-aggregatable metrics by encoding state in labels. This is a good approach to make metrics aggregatable. However, the implementation ofAddMetricValueFilter
function inpkg/query-service/app/metrics/query_builder.go
can be optimized. The function currently has repetitive code for different numeric types, which can be simplified.
4. pkg/query-service/app/metrics/v4/query_builder.go:21
- Draft comment:
TheAddMetricValueFilter
function has repetitive code for different numeric types. Consider using a type switch with a single case for all numeric types to reduce redundancy. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a new filter__value
to handle non-aggregatable metrics by encoding state in labels. This is a good approach to make metrics aggregatable. However, the implementation ofAddMetricValueFilter
function inpkg/query-service/app/metrics/query_builder.go
can be optimized. The function currently has repetitive code for different numeric types, which can be simplified.
5. pkg/query-service/app/inframetrics/namespaces.go:311
- Draft comment:
Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. This is applicable in multiple places in the code. - Reason this comment was not posted:
Comment was on unchanged code.
6. pkg/query-service/app/inframetrics/nodes.go:329
- Draft comment:
Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. This is applicable in multiple places in the code. - Reason this comment was not posted:
Marked as duplicate.
7. pkg/query-service/app/inframetrics/pods.go:371
- Draft comment:
Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. This is applicable in multiple places in the code. - Reason this comment was not posted:
Marked as duplicate.
8. pkg/query-service/app/inframetrics/pods_query.go:203
- Draft comment:
Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. This is applicable in multiple places in the code. - Reason this comment was not posted:
Marked as duplicate.
9. pkg/query-service/app/metrics/query_builder.go:12
- Draft comment:
Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. This is applicable in multiple places in the code. - Reason this comment was not posted:
Marked as duplicate.
10. pkg/query-service/app/metrics/v3/query_builder.go:339
- Draft comment:
Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. This is applicable in multiple places in the code. - Reason this comment was not posted:
Marked as duplicate.
11. pkg/query-service/app/metrics/v4/cumulative/table.go:23
- Draft comment:
Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. This is applicable in multiple places in the code. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_09H8UZX5ndxmPLIu
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
shivanshuraj1333
approved these changes
Dec 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Part of #5373
Certain metrics like pod phase encode the state of the pod in value. This makes them non-aggregatable. Ideally, they should encode the state in labels. To address this, I added a special filters
__value
, which can be used to filter out metrics with value in the query. So if you want to count the number of pods in running state, you can__value=2
. The same applies to node conditions. All the changes related toMetricValueFilter
are doing this.Change of
SpaceAggregationSum
toSpaceAggregationAvg
. This is because it not meaningful to sum up the utilization percentage values. Example, if you have 2 pods with 1 cpu request and they are using 2 cpu each, then the utilization as a percentage is 200% for both. If you sum up the utilization, you get 400%, which is not correct (because the total cpu request is 2 and the total cpu usage is 4, which is 200% utilization).Fix the map key for
cpu_request
andmemory_request
Add pod phase and node condition metrics to the default metrics list.
Important
Enhances Kubernetes monitoring by adding value-based metric filtering, adjusting aggregation methods, and including new metrics for pod phases and node conditions.
__value
filter inquery_builder.go
to filter metrics by value, applicable innodes_query.go
,pods_query.go
, andworkload_query.go
.SpaceAggregationSum
toSpaceAggregationAvg
inpods_query.go
,workload_query.go
, andquery_builder.go
to correctly calculate utilization percentages.namespaces.go
,nodes.go
, andpods.go
.NodeCountByCondition
andPodCountByPhase
toinfra.go
for enhanced metric tracking.cpu_request
andmemory_request
inworkload_query.go
.AddMetricValueFilter
function inquery_builder.go
to support new filtering logic.This description was created by for c795f58. It will automatically update as commits are pushed.