-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: add k8s {deployment, daemonset, statefulset, job} resources #6401
Conversation
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 9aa761f in 1 minute and 14 seconds
More details
- Looked at
782
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. pkg/query-service/app/inframetrics/common.go:76
- Draft comment:
ThegetParamsForTopDeployments
function is redundant as it only callsgetParamsForTopItems
. Consider removing it and usinggetParamsForTopItems
directly. - Reason this comment was not posted:
Confidence changes required:50%
ThegetParamsForTopDeployments
function inpkg/query-service/app/inframetrics/common.go
is redundant as it simply callsgetParamsForTopItems
. This pattern is repeated for other similar functions as well.
2. pkg/query-service/app/inframetrics/deployments.go:152
- Draft comment:
Modifyingreq.GroupBy
directly can lead to unexpected side effects. Consider creating a copy ofreq
before modification to avoid this issue. - Reason this comment was not posted:
Confidence changes required:50%
Inpkg/query-service/app/inframetrics/deployments.go
, thegetMetadataAttributes
function modifies thereq.GroupBy
parameter, which can lead to unexpected side effects if the request object is reused. This pattern is repeated in other similar functions as well.
3. pkg/query-service/app/inframetrics/deployments.go:255
- Draft comment:
Usingmath.Min
for limit calculation can lead to incorrect results ifreq.Offset
is greater than the length offormattedResponse[0].Series
. Consider adding a check to ensurereq.Offset
is within bounds. - Reason this comment was not posted:
Confidence changes required:50%
Inpkg/query-service/app/inframetrics/deployments.go
, thegetTopDeploymentGroups
function usesmath.Min
to calculate the limit, which can lead to incorrect results ifreq.Offset
is greater than the length offormattedResponse[0].Series
. This pattern is repeated in other similar functions as well.
4. pkg/query-service/app/inframetrics/workload_query.go:6
- Draft comment:
Ensure consistency in key naming betweenmetricNamesForWorkloads
andqueryNamesForDeployments
to avoid confusion and potential errors. - Reason this comment was not posted:
Confidence changes required:50%
Inpkg/query-service/app/inframetrics/workload_query.go
, themetricNamesForWorkloads
map uses inconsistent keys compared to thequeryNamesForDeployments
map indeployments.go
. This inconsistency can lead to confusion and potential errors.
5. pkg/query-service/app/http_handler.go:195
- Draft comment:
Do not add non-ClickHouse related functions to the ClickHouseReader interface. Use the DAO in the telemetry instance to access such information instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment does not appear to be related to any specific change in the diff. It seems to be a general guideline rather than a comment on a specific code change. The diff does not show any modifications to the ClickHouseReader interface or any indication that non-ClickHouse related functions were added to it.
I might be missing some context about the ClickHouseReader interface or the overall architecture, but based on the diff provided, there is no indication that the comment is relevant to the changes made.
Given the lack of evidence in the diff, it is reasonable to conclude that the comment is not relevant to the changes made.
The comment should be deleted as it does not pertain to any specific change in the diff and seems to be a general guideline rather than a necessary code change.
Workflow ID: wflow_iMGQh8voK2nOrlap
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
lgtm, thanks!
Summary
Part of #5373
Important
Adds Kubernetes deployment metrics handling to the query service with new API routes and data models.
DeploymentsRepo
to handle Kubernetes deployment metrics indeployments.go
.http_handler.go
for deployment attribute keys, values, and list.DeploymentListRequest
andDeploymentListResponse
toinfra.go
.DeploymentListRecord
toinfra.go
.getParamsForTopDeployments()
incommon.go
for deployment queries.workload_query.go
.APIHandler
inhttp_handler.go
to includedeploymentsRepo
.This description was created by for 9aa761f. It will automatically update as commits are pushed.