-
Notifications
You must be signed in to change notification settings - Fork 33
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
Adding craft doc for RLP #292
Conversation
Codecov Report
@@ Coverage Diff @@
## main #292 +/- ##
==========================================
- Coverage 65.52% 64.55% -0.98%
==========================================
Files 35 35
Lines 3806 3806
==========================================
- Hits 2494 2457 -37
- Misses 1129 1155 +26
- Partials 183 194 +11
Flags with carried forward coverage won't be shown. Click here to find out 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.
I understand this is intended to be some RLP 101. However, IMHO, it overloads a bit with https://docs.kuadrant.io/kuadrant-operator/doc/rate-limiting; it adds new wording for fields and concepts more carefully defined in the Reference, as well as in https://docs.kuadrant.io/kuadrant-operator/doc/reference/route-selectors/.
I think bits of this doc could perhaps be extracted to enhance other existing docs – either the ones I linked here or some of the more basic user guides. But, honestly, I don't think we need it as-is as part of our documentation.
Not that the approach is not desirable. On the contrary! I'm sure first-time users could really use such a detailed explanation that slices down each block and level of the RLP. Maybe it could be turned into a blog post instead, with a context use case to drive the explanations?
doc/craft/ratelimitpolicy.md
Outdated
## Spec | ||
|
||
### limits | ||
`spec.limits` is a list of named limits, which describe what limits should be placed on a resource. |
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.
What does "resource" mean here?
doc/craft/ratelimitpolicy.md
Outdated
``` | ||
The above example creates a new counter for every url_path that is requested. | ||
|
||
A more concrete example of why creating new counters base off the well known attributes may relate to comments on social network. |
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.
Sorry. I don't understand this sentence.
``` | ||
The above example will match HTTP requests that are sent with the header HTTP_USER_AGENT set with a value of "curl/8.0.1". | ||
|
||
### targetRef |
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.
Maybe explain this before explaining routeSelectors
?
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.
I explained the different sections in an alphabetically order as this is how a user would see items listed in a CR. If this was to be ordered by the perceived importances should routeSelectors
be listed before counters
?
- fixing broken links in markdown views - reword some sections - spell issues - indent changed from 4 to 2 spaces.
f7363e4
to
14ee73e
Compare
@guicassolato Regarding your main comment. The overview in https://docs.kuadrant.io/kuadrant-operator/doc/rate-limiting does not go into enough detail and requires the fully reading to the doc to understand what is going on. It is a high level overview. The Reference does not give a detail description of what the different sections are and the TOC can not be used to find the different areas. As for the route-selector link that you mention while that might explain what I was looking for, it is not in the sites nav so I would never have found it without knowing it was there. Plus that does not explain what counters are, or give examples what what the CR should look like. I would be happy with expending existing documents but which ones. The overview does not need that level of details and reference doc in its current structure of tables does not allow its self to that level of detail either. On the point of doing this as a blog post. In my mind a blog post would tell a story on how to solve a use case, which would not need all the different configuration options of the RLP. Then there is the question of where do you point the blog post? I would believe it is better to keep a user on the site over going to external sites. A blog post is a good idea as an accompanying artefact to the doc being added to the site. |
Sorry for the long reply, but too many topics to touch on...
I think that's the idea of the overview, not to give too much details. For the details, readers should refer to the Reference.
I honestly think that that's fine and how an overview document is expected to be consumed. In fact, we even fail on that by having sections like "Implementation details" in that doc.
Maybe the Reference could be enhanced to better explain each part that composes a RLP, possibly aiming for something along the lines of https://gateway-api.sigs.k8s.io/reference/spec/.
Here I disagree. I think the TOC could not be a better summary of the structure of a RLP; perhaps only missing the
So let's add it? That document is common across RLP and AP. It is linked from both overview pages and from the API reference ones, but if you think it deserves a place of its own in the nav menu, I'm OK with that.
I think the Reference doc is the one to enhance. This doc is not supposed to be a "quick reference" but the one doc that details every single field that shall or can be specified in a RLP resource – beyond what typically could be obtained from a
It says: "Each distinct value resolved in the data plane starts a separate counter for each rate limit." We can try better wording and examples if that's too cryptic, which I'm willing to accept. Again, maybe Gateway API's API specification page can be a good reference to follow here.
I'm glad we're in agreement about this. Moreover, I see the doc proposed here as an attempt to take the user by the hand and help get through the writing of a RLP for the very first time. This is the kind of thing that (for most people) works better with a background user story, an actual problem to solve and that drives the conversation. About where to link the blob post, well that depends. It was proposed about having a page in the main website that links to our most recent publications. Some other times, it's good that posts are ephemeral and tend to fade away naturally when no longer relevant. I think it's really about what you want to achieve and how committed you are to maintain that throughout time. As a final note, I 100% believe that, with a decent overview page that explains what the heck a RLP is for, plus a detailed API Reference where we don't miss a field, users will learn mainly through examples. There are how-to guides that are thought for basic users, which can contain additional bits about the whys of this and that, and there are other guides for more advanced use cases. We have of both kinds. We can work on all of those things (overview, reference and how-to guides) and not add more files to maintain IMO. |
@guicassolato and I had a good conversation about this change. We are in agreement that the information contained in the proposed doc is valuable but adding this information by this doc might not be the best way. The next step is to review this content with the aim of refactoring it into the existing documents (overview, reference and how-to guides). Marking this PR as a draft for now. |
Add a more detail guide to for the sections of a RLP.
Verification
As the change is to the docs and the main mode of render will be the docs.kuadrant.io there is a draft PR opened to the docs site which includes these changes. That PR can be found here Kuadrant/docs.kuadrant.io#39
Expected: