chore: add k8s statefulset resources #6409
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
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to c3d32c6 in 51 seconds
More details
- Looked at
612lines of code in5files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. pkg/query-service/app/http_handler.go:119
- Draft comment:
Ensure that the newstatefulsetsRepois properly initialized and used consistently with other similar repos likedeploymentsRepoanddaemonsetsRepo. This includes checking for any missing initialization or usage in other parts of the codebase. - Reason this comment was not posted:
Confidence changes required:20%
The code for handling stateful sets is consistent with the existing pattern for other Kubernetes resources like deployments and daemon sets. The new methods for stateful sets are added to the APIHandler struct, and the corresponding routes are registered. The implementation of the methods follows the same pattern as the existing ones, ensuring consistency and maintainability.
2. pkg/query-service/app/inframetrics/common.go:84
- Draft comment:
Ensure that thegetParamsForTopStatefulSetsfunction is used correctly in the context of stateful sets, similar to how othergetParamsForTop*functions are used for their respective resources. - Reason this comment was not posted:
Confidence changes required:10%
The functiongetParamsForTopStatefulSetsis consistent with other similar functions for different Kubernetes resources. It uses thegetParamsForTopItemsfunction to determine the step and table names based on the time range, which is a common pattern in this codebase.
3. pkg/query-service/app/inframetrics/statefulsets.go:271
- Draft comment:
Ensure that theGetStatefulSetListmethod handles all edge cases, such as empty responses or invalid request parameters, similar to other list methods for different resources. - Reason this comment was not posted:
Confidence changes required:20%
TheGetStatefulSetListmethod inStatefulSetsRepois consistent with other similar methods for different Kubernetes resources. It handles the request parameters, constructs the query, and processes the response in a similar manner.
4. pkg/query-service/app/http_handler.go:119
- 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:
Comment was on unchanged code.
Workflow ID: wflow_duNTcRrwqveE8eBp
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -412,6 +415,11 @@ func (aH *APIHandler) RegisterInfraMetricsRoutes(router *mux.Router, am *AuthMid | |||
| daemonsetsSubRouter.HandleFunc("/attribute_keys", am.ViewAccess(aH.getDaemonSetAttributeKeys)).Methods(http.MethodGet) | |||
| daemonsetsSubRouter.HandleFunc("/attribute_values", am.ViewAccess(aH.getDaemonSetAttributeValues)).Methods(http.MethodGet) | |||
| daemonsetsSubRouter.HandleFunc("/list", am.ViewAccess(aH.getDaemonSetList)).Methods(http.MethodPost) | |||
|
|
|||
| statefulsetsSubRouter := router.PathPrefix("/api/v1/statefulsets").Subrouter() | |||
| statefulsetsSubRouter.HandleFunc("/attribute_keys", am.ViewAccess(aH.getStatefulSetAttributeKeys)).Methods(http.MethodGet) | |||
There was a problem hiding this comment.
having "_" to concatenate strings for API is not a good pattern, use "-" instead
There was a problem hiding this comment.
I have generally seen _ commonly. I don't know why it's not a good pattern. Stripe API example https://docs.stripe.com/api/customer_sessions?lang=curl
There was a problem hiding this comment.
this is from past experience, it's okay if stripe is using it, but if we can avoid it, it'd be better.
this is a nit comment so it's fine to me either ways
The statement that using underscores (_) for concatenating strings in APIs is not a good pattern, and that dashes (-) should be used instead, is context-dependent and can be both correct and incorrect based on various factors such as conventions, language-specific practices, or industry standards.
Here are some general guidelines:
1. REST API and URL Conventions:
In the context of REST APIs, the use of dashes (-) is generally preferred for separating words in URL paths. This is primarily for readability and SEO (Search Engine Optimization). For example:
Good: /get-user-details/
Bad: /get_user_details/
Dashes are often seen as more human-readable than underscores, especially in URLs, and are more consistent with how web addresses are written.
2. Snake Case vs. Kebab Case:
Snake case (snake_case): This is commonly used in programming languages like Python and in certain API parameter names.
Kebab case (kebab-case): This is more commonly used in URLs and some JavaScript-related APIs, as mentioned above.
Snake_case (underscore) is typically used in API parameters or JSON keys, whereas kebab-case (dash) is more common in URLs.
3. Consistency with Standards:
Some popular web frameworks or platforms, such as GraphQL or RESTful APIs, have different conventions. For example, RESTful paths generally use kebab-case (/user-profile/), while JSON payloads might use snake_case (user_profile).
If you're working within a specific platform, it's always a good idea to follow their established conventions.
4. Language and Framework Preferences:
Python, for example, tends to prefer snake_case in code, so API parameters might often use underscores (_).
JavaScript and Ruby on Rails often use kebab-case (-) for URLs and routes.
Summary:
Dashes (-) are preferred for separating words in URLs (REST API paths).
Underscores (_) are more commonly used in programming for variable names (snake_case), but are less favored in URLs.
The choice depends on the context (URLs vs. parameters vs. variable names) and the established conventions for your specific project or language ecosystem.
If you're building an API with clean and standard URL paths, dashes are usually better for readability and consistency with web standards.
There was a problem hiding this comment.
Can you share the source for this?
Comment on lines
+32
to
+42
| queryNamesForStatefulSets = map[string][]string{ | ||
| "cpu": {"A"}, | ||
| "cpu_request": {"B", "A"}, | ||
| "cpu_limit": {"C", "A"}, | ||
| "memory": {"D"}, | ||
| "memory_request": {"E", "D"}, | ||
| "memory_limit": {"F", "D"}, | ||
| "restarts": {"G", "A"}, | ||
| "desired_pods": {"H"}, | ||
| "available_pods": {"I"}, | ||
| } |
There was a problem hiding this comment.
same comment as previous PR
| } | ||
|
|
||
| func (d *StatefulSetsRepo) getMetadataAttributes(ctx context.Context, req model.StatefulSetListRequest) (map[string]map[string]string, error) { | ||
| statefulSetAttrs := map[string]map[string]string{} |
There was a problem hiding this comment.
see if composite keys can work here in the logic.
eg
data["key1:key2"] = "value"
There was a problem hiding this comment.
I needed to use them so it is easier to main the separate map.
shivanshuraj1333
approved these changes
Nov 9, 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
Important
Add support for Kubernetes StatefulSets in the query service, including new API endpoints, repository methods, and model definitions.
http_handler.goandinfra.gowith new API endpoints for attribute keys, values, and list.StatefulSetsRepoinstatefulsets.goto handle data retrieval for StatefulSets.StatefulSetListRequest,StatefulSetListResponse, andStatefulSetListRecordtoinfra.go.getParamsForTopStatefulSets()incommon.gofor query parameter handling.statefulsets.go.This description was created by
for c3d32c6. It will automatically update as commits are pushed.