-
Notifications
You must be signed in to change notification settings - Fork 195
Expand Circuit Breaker Troubleshooting #4520
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
Vale Linting ResultsSummary: 1 warning, 4 suggestions found
|
| File | Line | Rule | Message |
|---|---|---|---|
| troubleshoot/elasticsearch/circuit-breaker-errors.md | 27 | Elastic.Articles | Use 'an HTTP' instead of 'a HTTP'. The article depends on pronunciation, not spelling. |
💡 Suggestions (4)
| File | Line | Rule | Message |
|---|---|---|---|
| troubleshoot/elasticsearch/circuit-breaker-errors.md | 27 | Elastic.Wordiness | Consider using 'to' instead of 'in order to'. |
| troubleshoot/elasticsearch/circuit-breaker-errors.md | 61 | Elastic.Wordiness | Consider using 'because' instead of 'since'. |
| troubleshoot/elasticsearch/circuit-breaker-errors.md | 116 | Elastic.WordChoice | Consider using 'can, might' instead of 'may', unless the term is in the UI. |
| troubleshoot/elasticsearch/circuit-breaker-errors.md | 119 | Elastic.WordChoice | Consider using 'can, might' instead of 'may', unless the term is in the UI. |
yetanothertw
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.
Thanks so much for your contribution!
I've left a few suggestions to fix the broken links which are failing the build. Let me know if that works.
|
|
||
| ```console | ||
| GET _nodes/stats?filter_path=nodes.*.breakers | ||
| ``` |
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.
@tanja-milicic an optional good add would be from this request implemented here starting in 9.3.0 users will have a CAT Circuit Breakers API which will make troubleshooting this much faster.
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.
Nice! That is definitely much cleaner than current approach.
How would we add that since it is not available yet? Would it be better to add the change once we have 9.3.0 released?
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.
You can add content for a specific 9.x version by tagging it with the applies_to tagging system.
You can add an admonition that uses the tags or if you're adding more information than would fit a note, you can add a new section and tag that section.
When you add the tags, you don't have to worry if a version's been released or not (for example, you don't have to wait for 9.3 to go out) -- the docs system will take care of that and adjust the way it displays the tagged content.
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.
If you want to go ahead and add the 9.3-specific content, I'll help you add the correct tags 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.
Thank you! Would something like this work? af30eb1 - I have used {applies_to}stack: ga 9.3
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.
Okay, I see! I think in this situation, the easiest option is to use an applies_to switch.
The result would look like different tabs for your different versions:
The tab for 9.0 - 9.2:

Here's the syntax I used:
::::{applies-switch}
:::{applies-item} { "stack": "ga" }
Use the get node statistics API:
GET _nodes/stats?filter_path=nodes.*.breakers:::
:::{applies-item} { "stack": "ga 9.3" }
Use the get circuit breakers statistics API:
GET /_cat/circuit_breaker/:::
::::
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 I can just push this commit to your branch if you don't mind? Might be a bit easier 😅
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 Tanja's off now for the day but if I may vote, please go ahead.
However FWIW, v9.3.0 is future release today but likely by the time this PR merges will be live. Or we can merge without my FR as is & then add this in a couple weeks. But in a couple weeks, the same question will need answered that <9.3.0 has to use the Node Stats API and ≥9.3.0 can use the CAT version. How could we do that without tabs but still mark the version differences?
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 pushed the tabs-related syntax, but now I'm not so sure the tabs work as they would imply the two API calls are mutually exclusive (which they aren't as far as I can tell, because the Node Stats API will still be useable, right? It's not being deprecated or anything?).
Let me try just listing the two requests as options with in-line applies_to syntax.
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've done that in 1887f6e. I've listed both options and included descriptions for what each response will provide. Could you please review for accuracy?
|
|
||
| ## Circuit Breaker Types | ||
|
|
||
| [Circuit breaker types](elasticsearch://reference/elasticsearch/configuration-reference/circuit-breaker-settings.md) are manually defined for known expensive code paths with a sum-up [parent circuit breaker](elasticsearch://reference/elasticsearch/configuration-reference/circuit-breaker-settings.md#parent-circuit-breaker). Breakers need to be resolved per breaker type; common examples: |
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 content is excellent but I think should remain in the first linked reference page of Circuit Breaker Settings. Although it should be expanded if any of these bullet points are missing?
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.
Good point! I have removed it as most of it is in the Circuit breaker settings page. However, I see that we removed Accounting requests circuit breaker from the docs - it is available up until 8.13 version. I can't find the reason why it was removed. Any idea?
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.
Hi @tanja-milicic, you can still find it for the 8.13 version of the docs here: https://www.elastic.co/guide/en/elasticsearch/reference/8.13/circuit-breaker.html#accounting-circuit-breaker
We're using a different versioning system for 9.x docs (which is the file you're editing now); it's a cumulative docs approach.
Do you need to edit the 8.13 docs? I don't think we maintain the docs that far back, but I can ask my colleagues for ideas on how to proceed.
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.
Thank you! I wanted to check why the section was removed and if we really don't need it anymore. I am not able to find PR that removed 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.
I think that would be this PR here. Note that it's located in the elasticsearch repo (as opposed to the docs-content repo that hosts 9.x content) on the 8.14 branch -- the 8.x content and branching strategy for docs is/was different.
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.
Ah right, thank you for explaining it!
Co-authored-by: Vlada Chirmicci <vlada.chirmicci@elastic.co>
🔍 Preview links for changed docs |
stefnestor
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.
💚
yetanothertw
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.
Thank you for your help! 💟

Ports Circuit Breakers article to docs.
Summary
Adds:
Generative AI disclosure
cc @stefnestor