-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
stats: Remember recent lookups and display them in an admin endpoint #8116
stats: Remember recent lookups and display them in an admin endpoint #8116
Conversation
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
This is ready for a round of review, though I realize I may need to work a little more on the stats.md doc. |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
I have an un-pushed change on top of this that adds logging. The extra code is small (~ 300 lines) but the implication is risk of spamming logs. Currently I have this set to dump the remembered (10) recent lookups for the last 5 minutes, at most once every 5 minutes. I am wondering if that should be flag-controlled and if therefore we should review this as is, and then let the logging addition be a follow-up. I'm also thinking that a config API to add builtin stat-names would make any reports in those logs actionable by Envoy users without having to make code changes. Maybe that should all be integrated into this one PR? WDTY? |
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
…r to gauge. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
…k for this! Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
/retest |
🔨 rebuilding |
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 this looks great, just some small nits.
/wait
docs/root/operations/admin.rst
Outdated
but in response to user requests on high core-count machines, this | ||
can cause performance issues due to mutex contention. | ||
|
||
This option requires Envoy to be started with `use-fake-symbol-table 0`. |
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.
nit: I think you can use an :option
ref link here.
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.
done; thanks.
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.
Actually I backed this out, as use-fake-symbol-table
is not docced, as it was meant to be temporary.
docs/root/operations/admin.rst
Outdated
.. http:post:: /stats/recentlookups/clear | ||
|
||
Clears all outstanding lookups and counts. If called when recent lookup | ||
collection is enabled, this clears all the, but collection |
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.
typo
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.
fixed, and I simplified this, as I don't think it adds anything to the paragraph to discuss the two different cases.
commit coming.
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
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!
…nvoyproxy#8116) * stats: Remember recent lookups and display them in an admin endpoint Signed-off-by: Joshua Marantz <jmarantz@google.com>
Description: Adds an admin endpoint to show recent lookups. This blocks #4980 .
Risk Level: medium -- this does add overhead when creating new StatNames.
Testing: //test/...
Docs Changes: yes, in the PR.
Release Notes: in the PR.
Fixes: #8035