Skip to content
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 daemonset resources #6408

Merged
merged 2 commits into from
Nov 9, 2024
Merged

Conversation

srikanthccv
Copy link
Member

@srikanthccv srikanthccv commented Nov 8, 2024

Summary

Part of #5373


Important

Add support for Kubernetes DaemonSets in query service with new API routes, repository methods, and models.

  • Behavior:
    • Adds support for Kubernetes DaemonSets in APIHandler with new routes for attribute keys, values, and list in http_handler.go and infra.go.
    • Introduces DaemonSetsRepo in daemonsets.go to handle DaemonSet metrics and attributes.
  • Models:
    • Adds DaemonSetListRequest, DaemonSetListResponse, and DaemonSetListRecord to infra.go.
  • Misc:
    • Adds getParamsForTopDaemonSets() in common.go for query parameter handling.
    • Updates APIHandler to include daemonsetsRepo in http_handler.go.

This description was created by Ellipsis for 533bbbf. It will automatically update as commits are pushed.

@github-actions github-actions bot added the chore label Nov 8, 2024
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 533bbbf in 1 minute and 12 seconds

More details
  • Looked at 608 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. pkg/query-service/app/inframetrics/daemonsets.go:100
  • Draft comment:
    Consider removing the hardcoded metric name k8s_pod_cpu_utilization and support keys from any pod metric. This will make the code more flexible and adaptable to changes in metric names.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code in the PR seems to be consistent with the existing pattern for handling different Kubernetes resources. However, there is a potential issue with the use of hardcoded metric names and the lack of error handling for certain operations.
2. pkg/query-service/app/inframetrics/daemonsets.go:112
  • Draft comment:
    Ensure that only resource attributes are returned when possible. This will help in maintaining consistency and clarity in the data being processed.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code in the PR seems to be consistent with the existing pattern for handling different Kubernetes resources. However, there is a potential issue with the use of hardcoded metric names and the lack of error handling for certain operations.
3. pkg/query-service/app/inframetrics/daemonsets.go:171
  • Draft comment:
    Ensure that the query replacement from local to distributed is correct and does not affect the intended query logic.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code in the PR seems to be consistent with the existing pattern for handling different Kubernetes resources. However, there is a potential issue with the use of hardcoded metric names and the lack of error handling for certain operations.
4. pkg/query-service/app/inframetrics/daemonsets.go:245
  • Draft comment:
    Consider handling cases where req.OrderBy.ColumnName might not exist in queryNamesForDaemonSets. This will prevent potential runtime errors.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code in the PR seems to be consistent with the existing pattern for handling different Kubernetes resources. However, there is a potential issue with the use of hardcoded metric names and the lack of error handling for certain operations.
5. pkg/query-service/app/http_handler.go:123
  • Draft comment:
    Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. This comment applies to the entire file structure, not just this line.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code is adding a new repository for DaemonSets in the APIHandler, which is consistent with the existing pattern for other resources like deployments, clusters, etc. This is appropriate and does not violate any of the specified rules.

Workflow ID: wflow_pYCvNn15AFCIuqOZ


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

pkg/query-service/app/inframetrics/common.go Show resolved Hide resolved
@srikanthccv srikanthccv changed the title chore: add k8s deployment resources chore: add k8s daemonset resources Nov 8, 2024
@srikanthccv srikanthccv merged commit 5bdd43e into add-deployment Nov 9, 2024
7 checks passed
@srikanthccv srikanthccv deleted the add-daemonsets branch November 9, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants