Design doc updates for Pod Count#1923
Open
khansaad wants to merge 2 commits into
Open
Conversation
Signed-off-by: Saad Khan <saakhan@ibm.com>
Contributor
Reviewer's GuideAdds documentation for a new unified Recommendations API endpoint (used by the Pod count feature) to the MonitoringMode API design doc, including request parameters, example usage, a detailed sample response showing pod_count metrics and pods_count recommendations, and a table of possible error responses. Sequence diagram for the unified Recommendations API endpointsequenceDiagram
actor User
participant RecommendationsAPI
User->>RecommendationsAPI: POST /kruize/api/v1/recommendations
alt [missing experiment_name]
RecommendationsAPI-->>User: 400 experiment_name is mandatory
else [remote mode without interval_end_time]
RecommendationsAPI-->>User: 400 interval_end_time is mandatory
else [invalid timestamp format]
RecommendationsAPI-->>User: 400 Given timestamp is not a valid timestamp format
else [no metrics or invalid interval]
RecommendationsAPI-->>User: 400 No metrics or invalid interval
else [valid request]
RecommendationsAPI-->>User: 201 recommendations with pod_count and pods_count
end
Flow diagram for Recommendations API request validation and pod count-based recommendationsflowchart TD
Start([Start /kruize/api/v1/recommendations request])
VExp{experiment_name provided?}
VEndReq{Remote mode
requires interval_end_time?}
VEndProvided{interval_end_time provided?}
VFormat{Timestamps valid
and well formatted?}
VOrder{interval_start_time
<= interval_end_time?}
VRange{Range <= 15 days?}
Success([Generate recommendations
with pod_count metrics
201 Created])
Err400Exp([400 experiment_name is mandatory])
Err400End([400 interval_end_time is mandatory])
Err400Format([400 invalid timestamp format])
Err400Order([400 The Start time should precede the End time])
Err400Range([400 The gap must be within a maximum of 15 days])
Start --> VExp
VExp -->|No| Err400Exp
VExp -->|Yes| VEndReq
VEndReq -->|Local mode| VFormat
VEndReq -->|Remote mode| VEndProvided
VEndProvided -->|No| Err400End
VEndProvided -->|Yes| VFormat
VFormat -->|No| Err400Format
VFormat -->|Yes| VOrder
VOrder -->|No| Err400Order
VOrder -->|Yes| VRange
VRange -->|No| Err400Range
VRange -->|Yes| Success
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
12 tasks
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The timestamp format string is documented as
yyyy-MM-ddTHH:mm:sssZ; consider clarifying or correcting this to the conventionalyyyy-MM-dd'T'HH:mm:ss.SSS'Z'(or similar) so users know exactly what format the API expects. - In the example response,
metrics_infousespod_countwhile the recommendation engines usepods_count; it would help to standardize this naming or explicitly call out the difference to avoid confusion for consumers. - The error responses table lists multiple 400 entries with similar phrasing (e.g., two mandatory-parameter rows and a trailing
|on the last-but-one line); consolidating or tightening these entries would make the error semantics clearer and the table formatting more consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The timestamp format string is documented as `yyyy-MM-ddTHH:mm:sssZ`; consider clarifying or correcting this to the conventional `yyyy-MM-dd'T'HH:mm:ss.SSS'Z'` (or similar) so users know exactly what format the API expects.
- In the example response, `metrics_info` uses `pod_count` while the recommendation engines use `pods_count`; it would help to standardize this naming or explicitly call out the difference to avoid confusion for consumers.
- The error responses table lists multiple 400 entries with similar phrasing (e.g., two mandatory-parameter rows and a trailing `|` on the last-but-one line); consolidating or tightening these entries would make the error semantics clearer and the table formatting more consistent.
## Individual Comments
### Comment 1
<location path="design/MonitoringModeAPI.md" line_range="6803" />
<code_context>
+### Recommendations API
+
+**Note: This API is common for both Local and Remote Monitoring use case.** <br>
+**Note: This API will replace the existing updateRecommendations and generateRecommendations API as a single endpoint** <br>
+Generates the recommendation for a specific experiment based on provided parameters similar to updateRecommendations and generateRecommendations API.
</code_context>
<issue_to_address>
**suggestion (typo):** Consider using the plural form "use cases" here.
Because this note covers both Local and Remote Monitoring, the plural "use cases" is grammatically correct here.
```suggestion
**Note: This API is common for both Local and Remote Monitoring use cases.** <br>
```
</issue_to_address>
### Comment 2
<location path="design/MonitoringModeAPI.md" line_range="6804-6805" />
<code_context>
+### Recommendations API
+
+**Note: This API is common for both Local and Remote Monitoring use case.** <br>
+**Note: This API will replace the existing updateRecommendations and generateRecommendations API as a single endpoint** <br>
+Generates the recommendation for a specific experiment based on provided parameters similar to updateRecommendations and generateRecommendations API.
+This can be called based on the mode which user selects in the config.
</code_context>
<issue_to_address>
**suggestion (typo):** Pluralize "API" at the end to match the two APIs being referenced.
Since this sentence refers to both `updateRecommendations` and `generateRecommendations`, the ending should use "APIs" (e.g., "generateRecommendations APIs") rather than the singular "API."
```suggestion
**Note: This API will replace the existing updateRecommendations and generateRecommendations APIs as a single endpoint** <br>
Generates the recommendation for a specific experiment based on provided parameters similar to updateRecommendations and generateRecommendations APIs.
```
</issue_to_address>
### Comment 3
<location path="design/MonitoringModeAPI.md" line_range="6805-6806" />
<code_context>
+
+**Note: This API is common for both Local and Remote Monitoring use case.** <br>
+**Note: This API will replace the existing updateRecommendations and generateRecommendations API as a single endpoint** <br>
+Generates the recommendation for a specific experiment based on provided parameters similar to updateRecommendations and generateRecommendations API.
+This can be called based on the mode which user selects in the config.
+
+**Request Parameters**
</code_context>
<issue_to_address>
**suggestion (typo):** Improve grammar by adding a subject, pluralizing "recommendation", and adding "the" before "user".
For example, you could rewrite them as: "This API generates recommendations for a specific experiment based on the provided parameters, similar to the `updateRecommendations` and `generateRecommendations` APIs. It can be called based on the mode that the user selects in the config."
```suggestion
This API generates recommendations for a specific experiment based on the provided parameters, similar to the `updateRecommendations` and `generateRecommendations` APIs.
It can be called based on the mode that the user selects in the config.
```
</issue_to_address>
### Comment 4
<location path="design/MonitoringModeAPI.md" line_range="6816" />
<code_context>
+| interval_end_time | string | mandatory/optional | The end time of the interval in the format `yyyy-MM-ddTHH:mm:sssZ`. This should be the date on which recommendation needs to be generated. **Mandatory for remote mode** |
+| interval_start_time | string | optional | The start time of the interval in the format `yyyy-MM-ddTHH:mm:sssZ`. |
+
+The recommendation API requires only one mandatory field i.e. `experiment_name` in case of local mode and both `experiment_name` and `interval_end_time` in case of remote.
+Similarly, `interval_start_time` will be calculated based on `interval_end_time`, if not provided. By utilizing
+these parameters, the API generates recommendations based on short-term, medium-term, and long-term factors. For
</code_context>
<issue_to_address>
**nitpick (typo):** Tighten the grammar around "i.e." and clarify the modes.
Consider adding commas around “i.e.” and smoothing the sentence, for example: “The recommendation API requires only one mandatory field, i.e., `experiment_name`, in local mode, and both `experiment_name` and `interval_end_time` in remote mode.”
```suggestion
The recommendation API requires only one mandatory field, i.e., `experiment_name`, in local mode, and both `experiment_name` and `interval_end_time` in remote mode.
```
</issue_to_address>
### Comment 5
<location path="design/MonitoringModeAPI.md" line_range="6841" />
<code_context>
+
+**Response**
+
+The response will contain a array of JSON object with the recommendations for the specified experiment.
+
+<details>
</code_context>
<issue_to_address>
**issue (typo):** Fix article and pluralization in this sentence.
Please change this to: "an array of JSON objects."
```suggestion
The response will contain an array of JSON objects with the recommendations for the specified experiment.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Saad Khan <saakhan@ibm.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Description
This PR contains docs changes for the
Pod countfeature.Fixes # (issue)
Type of change
How has this been tested?
Please describe the tests that were run to verify your changes and steps to reproduce. Please specify any test configuration required.
Test Configuration
Checklist 🎯
Additional information
Include any additional information such as links, test results, screenshots here
Summary by Sourcery
Documentation: