-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Improvements and structure changes for autoscaling docs #2439
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
Conversation
|
/retest |
|
/approve If you |
485c78f to
e4feac1
Compare
markusthoemmes
left a comment
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'm no expert but I'm wondering if making the users click more to get to the respective chapters is doing more harm than it does good. I personally like docs like these, which are mostly setting centered, to be on a single page so I can CTRL-F search for what I need.
The idea is that each section should have a topic that's a self-explanatory user story, i.e. a user looking for how to configure a certain thing will go to the section for that thing, rather than needing to search through a lengthy page/doc to find it. |
vagababov
left a comment
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.
It also seems to miss the parameters I've added (activator-capacty and scale to zero pod retention), so we might need to reconcile with the head.
| {{< /tab >}} | ||
| {{< /tabs >}} | ||
| ## Global versus per-revision settings |
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.
This should probably go above the class, since it affects the choices made there as well.
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 think this fits here as well -- it provides more detail, but if users are happy with the first choice they found, it should work.
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 think this is why we need to be able to reuse / include content - so we can have a blurb about something in more than one place in the docs but maintain it from one file.
evankanderson
left a comment
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.
A few minor bits, but this looks pretty good right now.
I'm going to:
/hold cancel
But I'm going to wait on the LGTM until you get a chance to comment on at least the sample/alias question.
| * Disable scale to zero functionality for your cluster ([global configuration only](./scale-to-zero.md)). | ||
| * Configure the [type of metrics](./autoscaling-metrics.md) your Autoscaler consumes. | ||
| * Configure [concurrency limits](./concurrency.md) for applications. | ||
| * Try out the [Go Autoscale Sample App](./autoscale-go/README.md). |
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.
Do you see doing a similar move for other samples in the future (e.g. traffic splitting samples into the networking subsection)?
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.
Yep, I'd like to do this with all of them, it just makes more sense to me as a place where someone would look for it. WDYT?
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 don't think we've discussed it; I don't have an intrinsic problem with it, but it would be good to have a documented strategy.
| {{< /tab >}} | ||
| {{< /tabs >}} | ||
| ## Global versus per-revision settings |
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 think this fits here as well -- it provides more detail, but if users are happy with the first choice they found, it should work.
|
|
||
| ### Per-revision settings | ||
|
|
||
| Per-revision settings for autoscaling are configured by adding _annotations_ to a revision. |
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.
Are the annotation keys the same ones as are present in the ConfigMap?
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.
Nope, they're different afaik, it's explained in the section for each key.
| The possible metric types that can be configured per revision depend on the type of Autoscaler implementation you are using: | ||
|
|
||
| * The default KPA Autoscaler supports the `concurrency` and `rps` metrics. | ||
| * The HPA Autoscaler supports the `concurrency`, `rps` and `cpu` metrics. |
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.
Can you describe what these options mean?
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.
In particular, how concurrency and RPS differ.
Feel free to defer with a <!-- TODO: ... --> if you don't have bandwidth right now.
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.
Added a TODO note :) I think I had pretty much the same note previously and removed it because I knew I wouldn't get to it 😂
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.
A TODO lets someone else pick up the item if you don't get back to it. 😁
| --- | ||
|
|
||
| Concurrency determines the number of simultaneous requests that can be processed by each replica of an application at any given time. | ||
| <!-- this is where including files would be useful. We could create a concurrency global config module and insert it here, in the docs for metrics, and in the docs for targets. Showing the correct information each time instead of having it in one place with the per revision config jumbled in with it makes it easier to understand IMHO, and would mean users don't need to visit different pages or hunt for the same information for similar user stories @abrennan89.--> |
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.
Reference an issue in the website repo if this isn't working and you want it to?
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'll come back to it when I have more time :)
evankanderson
left a comment
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.
One more alias that needs updating!
Sorry for the delay on re-reviewing.
|
No problem, thanks for looking at this @evankanderson - should be finally ready now 😂 |
|
BTW, you don't need to squash your changes to the docs repo; Tide will do a squash merge automatically when it merges, so each PR becomes one commit. Leaving the commits un-squashed during review makes it easier to see what's changed since the last commit. (But I'll run through this one and approve in a moment.) |
evankanderson
left a comment
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.
/lgtm
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abrennan89, evankanderson, mattmoor The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Auto TLS verification used the `autoscale-go` sample, but it was moved in #2439, and not updated here.
Proposed Changes