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

Adding craft doc for RLP #292

Closed
wants to merge 2 commits into from
Closed

Conversation

Boomatang
Copy link
Contributor

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:

  • Guide explains all components in the RLP.
  • Link in the guides are crafted for use on the site and may not work as expected if the Markdown file is view in isolation.

@Boomatang Boomatang requested a review from a team as a code owner November 7, 2023 10:24
Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Merging #292 (14ee73e) into main (bb6476e) will decrease coverage by 0.98%.
Report is 3 commits behind head on main.
The diff coverage is n/a.

@@            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     
Flag Coverage Δ
integration 70.09% <ø> (-1.82%) ⬇️
unit 58.20% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
api/v1beta1 (u) ∅ <ø> (∅)
pkg/common (u) 73.92% <ø> (ø)
pkg/istio (u) 30.24% <ø> (ø)
pkg/log (u) 31.81% <ø> (ø)
pkg/reconcilers (u) 33.68% <ø> (ø)
pkg/rlptools (u) 56.41% <ø> (ø)
controllers (i) 70.09% <ø> (-1.82%) ⬇️

see 8 files with indirect coverage changes

Copy link
Contributor

@guicassolato guicassolato left a 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 Show resolved Hide resolved
doc/craft/ratelimitpolicy.md Outdated Show resolved Hide resolved
doc/craft/ratelimitpolicy.md Outdated Show resolved Hide resolved
doc/craft/ratelimitpolicy.md Outdated Show resolved Hide resolved
## Spec

### limits
`spec.limits` is a list of named limits, which describe what limits should be placed on a resource.
Copy link
Contributor

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 Show resolved Hide resolved
```
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.
Copy link
Contributor

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.

doc/craft/ratelimitpolicy.md Outdated Show resolved Hide resolved
```
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
Copy link
Contributor

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?

Copy link
Contributor Author

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?

doc/craft/ratelimitpolicy.md Outdated Show resolved Hide resolved
- fixing broken links in markdown views
- reword some sections
- spell issues
- indent changed from 4 to 2 spaces.
@Boomatang
Copy link
Contributor Author

@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.

@guicassolato
Copy link
Contributor

Sorry for the long reply, but too many topics to touch on...


The overview in https://docs.kuadrant.io/kuadrant-operator/doc/rate-limiting does not go into enough detail

I think that's the idea of the overview, not to give too much details. For the details, readers should refer to the Reference.

requires the fully reading to the doc to understand

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.

The Reference does not give a detail description of what the different sections are

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/.

and the TOC can not be used to find the different areas

Here I disagree. I think the TOC could not be a better summary of the structure of a RLP; perhaps only missing the metadata bit, which arguably can be assumed to be preexisting knowledge for one who writes a Kubernetes resource and therefore only worth mentioning because of the same namespace constraint and (not event sure!) the added labels.

the route-selector link that you mention […] is not in the sites nav

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 would be happy with expending existing documents but which ones.

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 kubectl explain command.

Plus that does not explain what counters are

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.

a blog post would tell a story on how to solve a use case […] A blog post is a good idea as an accompanying artefact to the doc being added to the site.

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.

@Boomatang
Copy link
Contributor Author

@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.

@Boomatang Boomatang marked this pull request as draft November 15, 2023 12:47
@Boomatang Boomatang closed this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants