-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Fixes session idle timeout #91070
Fixes session idle timeout #91070
Conversation
Seen on all pages.
Seen on all pages.
Seen on page: /app/management/data/index_management
Seen on page: /app/management/insightsAndAlerting/watcher/watches
Seen on page: /app/management/data/rollup_jobs/job_list
Seen on page: /app/management/data/cross_cluster_replication/follower_indices
Seen on page: /app/management/data/cross_cluster_replication/auto_follow_patterns
…ms/_stats` Seen on page: /app/management/data/transform
2e69fe8
to
e756e91
Compare
Seen on page: /app/dev_tools#/console
…asticsearch_settings/check/cluster` Seen on page: /app/monitoring
getAll(): Promise<Tag[]>; | ||
getAll(options?: GetAllTagsOptions): Promise<Tag[]>; |
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 interface is also used by the server-side
export class TagsClient implements ITagsClient { |
and I think this option doesn't make any sense on the server.
But refactoring the code to use distinct interfaces is probably going to be a pain, so I'm ok keeping these changes for now as we do want that for 7.12
.
@elasticmachine merge upstream |
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 for making those changes! I left a comment in our use_request
hook that I think needs to be addressed. Cheers!
// Surface to consumers that at least one request has resolved. | ||
isInitialRequestRef.current = false; | ||
const resendRequest = useCallback( | ||
async (asSystemRequest?: boolean) => { |
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 it would be better to extract the parameter from UseRequestConfig
above in the useRequest
call and use it here. As we are not passing it on L120 and we don't want the consumer to change the request configuration when calling resendRequest
.
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 for the terrible diff here, all I did was 1. add an asSystemRequest
parameter to the anonymous callback function, and 2. construct the request payload using this new parameter.
Actually I don't think we want to expose this in UseRequestConfig
-- I see it as an implementation detail. A consumer calls the useRequest
function, which schedules itself to poll in the background. Consumers should not be able to determine whether this is treated as a system request. It's background polling, so it must be treated as such.
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.
OK I understand. But then when we provide that handler to the consumer should we protect it and set to false
on L145, or is it overkill?
const resendRequestForConsumer = useCallback(() => {
return resendRequest(false);
}, [resendRequest]);
return {
isInitialRequest: isInitialRequestRef.current,
isLoading,
error,
data,
resendRequest: resendRequestForConsumer, // Gives the user the ability to manually request data
};
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.
Oh, now I understand now what you were trying to say 😄
It's not something we need to protect. As in, there's no danger of other unintended consequences if a consumer happened to use this parameter.
However, we don't have any consumers that require the ability to use this when manually requesting data. With that in mind, I think we should apply your suggestion until we have a reason to expose it.
Edit: changed in 1ef7e5a.
|
||
const scheduleRequest = useCallback(() => { | ||
// If there's a scheduled poll request, this new one will supersede it. | ||
clearPollInterval(); | ||
|
||
if (pollIntervalMs) { | ||
pollIntervalIdRef.current = setTimeout(resendRequest, pollIntervalMs); | ||
pollIntervalIdRef.current = setTimeout(() => resendRequest(true), pollIntervalMs); |
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 am not sure I follow. The consumer can decide to pass it to the config (SendRequestConfig
in "send_request.ts") but here we force it to true
? Do you mind explaining? Do we always want it to be true
?
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.
As discussed in #91070 (comment), if this request is sent during background polling, it should be treated as a system request. Consumers should not be able to change that behavior.
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.
@jportner Sorry, I don't see a discussion at the link you shared. Could you help me understand your line of reasoning? What's the relationship between a poll and a system request? FWIW, the "poll" in this case simply defines behavior -- the request is resent automatically on an interval. This logic is otherwise unconcerned about the consumer's use case.
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, clipboard must have gotten screwed up. I edited the link to fix it. I was referring to my other comment above.
Let me rephrase: if a request can be sent (or resent) in the background, without user interaction, it should be flagged as a system request. Polling-style behavior such as this fits the bill.
Edit: to clarify, I did not see it as necessary to flag the initial request as a system request, since it's simply sent in the useEffect
hook when the page is initially rendered. I think all subsequent polling-style requests should be flagged as system requests, though.
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 Joe! I just read the linked issue in the PR description, which I should have done in the first place. I think I understand what you're trying to do now. We want to detect when a session has become idle. We're using requests to the server as a proxy for user interaction. Requests that are sent on a poll are incorrectly interpreted as user interaction, preventing idle sessions from being detected. asSystemRequest
enables the server to ignore these and similar requests, as a solution the problem.
I think this approach makes sense. My only concern is that the code doesn't communicate this intention, and possibly signals the wrong intentions because of the term "system". We have system indices in Elasticsearch, which carry connotations about restricted access and usage. In this case, "system" feels like too broad and powerful a term for what is a much narrower and less consequential scope. Can we find a name that matches this narrower scope? For example, asIdleRequest
?
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.
...
asSystemRequest
enables the server to ignore these and similar requests, as a solution the problem.
Yes!
I think this approach makes sense.
...
Can we find a name that matches this narrower scope? For example,asIdleRequest
?
The kbn-system-request
header has been around for a long time. I agree that calling this a "system request" is a terrible name, I didn't (and wouldn't) choose it 😄
So, the http plugin exposes its functions with the HttpFetchOptions
parameter, that has the asSystemRequest
field and sets the kbn-system-request
header accordingly.
We could change the parameter name to something else in these useRequest
and sendRequest
functions, but I am sort of concerned that would cause even more confusion that something like asIdleRequest
would control this header. Especially if/when we are trying to find all usages of the kbn-system-request
header in the future, this would make that more difficult.
How would you feel if I leave the name as-is, and add some comments in these two functions to explain the rationale, and add a TSdoc in the SendRequestConfig
interface?
Edit: I added some comments in 1ef7e5a. Let me know what you think, if you still have misgivings I'll change the parameter name as suggested.
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.
concerned that would cause even more confusion
Good point I agree.
How would you feel if I leave the name as-is, and add some comments in these two functions to explain the rationale, and add a TSdoc in the SendRequestConfig interface?
This sounds like a great solution. Thanks for offering it up. :)
added some comments
I think these are very helpful! Thank you so much for hearing me out, even though I should have spent more time familiarizing myself with the PR before commenting. 😅
x-pack/plugins/monitoring/public/lib/elasticsearch_settings/checkers/settings_checker.js
Outdated
Show resolved
Hide resolved
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.
Transform changes LGTM
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, thanks!
export async function loadClusters() { | ||
return await sendGet(); | ||
export async function loadClusters(options) { | ||
return await sendGet(undefined, options); |
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.
optional nit: since path
is optional it'd probably make sense to include it to SendGetOptions
, but up to you and ES UI team.
And yeah, great find that path
is actually optional even though it was defined as required in TS!
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 100% agree, I just didn't want to make a breaking change to the sendGet
method signature. I figured the ES UI team can always improve it in the future 😄
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.
Some .js + .ts "in between" state 😄
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
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 for making the changes and adding comments @jportner ! EsUI changes LGTM
* Fix calls to `/api/saved_objects_tagging/tags` Seen on all pages. * Fix calls to `/api/ui_counters/_report` Seen on all pages. * Fix calls to `/api/index_management/indices/reload` Seen on page: /app/management/data/index_management * Fix calls to `/api/watcher/watches` Seen on page: /app/management/insightsAndAlerting/watcher/watches * Fix calls to `/api/rollup/jobs` Seen on page: /app/management/data/rollup_jobs/job_list * Fix calls to `/api/cross_cluster_replication/follower_indices` Seen on page: /app/management/data/cross_cluster_replication/follower_indices * Fix calls to `/api/cross_cluster_replication/auto_follow_patterns` Seen on page: /app/management/data/cross_cluster_replication/auto_follow_patterns * Fix calls to `/api/transform/transforms` and `/api/transform/transforms/_stats` Seen on page: /app/management/data/transform * Fix calls to `/api/console/proxy` Seen on page: /app/dev_tools#/console * Fix calls to `/api/monitoring/v1/clusters` and `/api/monitoring/v1/elasticsearch_settings/check/cluster` Seen on page: /app/monitoring
* Fix calls to `/api/saved_objects_tagging/tags` Seen on all pages. * Fix calls to `/api/ui_counters/_report` Seen on all pages. * Fix calls to `/api/index_management/indices/reload` Seen on page: /app/management/data/index_management * Fix calls to `/api/watcher/watches` Seen on page: /app/management/insightsAndAlerting/watcher/watches * Fix calls to `/api/rollup/jobs` Seen on page: /app/management/data/rollup_jobs/job_list * Fix calls to `/api/cross_cluster_replication/follower_indices` Seen on page: /app/management/data/cross_cluster_replication/follower_indices * Fix calls to `/api/cross_cluster_replication/auto_follow_patterns` Seen on page: /app/management/data/cross_cluster_replication/auto_follow_patterns * Fix calls to `/api/transform/transforms` and `/api/transform/transforms/_stats` Seen on page: /app/management/data/transform * Fix calls to `/api/console/proxy` Seen on page: /app/dev_tools#/console * Fix calls to `/api/monitoring/v1/clusters` and `/api/monitoring/v1/elasticsearch_settings/check/cluster` Seen on page: /app/monitoring
Backport result
|
* Fix calls to `/api/saved_objects_tagging/tags` Seen on all pages. * Fix calls to `/api/ui_counters/_report` Seen on all pages. * Fix calls to `/api/index_management/indices/reload` Seen on page: /app/management/data/index_management * Fix calls to `/api/watcher/watches` Seen on page: /app/management/insightsAndAlerting/watcher/watches * Fix calls to `/api/rollup/jobs` Seen on page: /app/management/data/rollup_jobs/job_list * Fix calls to `/api/cross_cluster_replication/follower_indices` Seen on page: /app/management/data/cross_cluster_replication/follower_indices * Fix calls to `/api/cross_cluster_replication/auto_follow_patterns` Seen on page: /app/management/data/cross_cluster_replication/auto_follow_patterns * Fix calls to `/api/transform/transforms` and `/api/transform/transforms/_stats` Seen on page: /app/management/data/transform * Fix calls to `/api/console/proxy` Seen on page: /app/dev_tools#/console * Fix calls to `/api/monitoring/v1/clusters` and `/api/monitoring/v1/elasticsearch_settings/check/cluster` Seen on page: /app/monitoring Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>
* Fix calls to `/api/saved_objects_tagging/tags` Seen on all pages. * Fix calls to `/api/ui_counters/_report` Seen on all pages. * Fix calls to `/api/index_management/indices/reload` Seen on page: /app/management/data/index_management * Fix calls to `/api/watcher/watches` Seen on page: /app/management/insightsAndAlerting/watcher/watches * Fix calls to `/api/rollup/jobs` Seen on page: /app/management/data/rollup_jobs/job_list * Fix calls to `/api/cross_cluster_replication/follower_indices` Seen on page: /app/management/data/cross_cluster_replication/follower_indices * Fix calls to `/api/cross_cluster_replication/auto_follow_patterns` Seen on page: /app/management/data/cross_cluster_replication/auto_follow_patterns * Fix calls to `/api/transform/transforms` and `/api/transform/transforms/_stats` Seen on page: /app/management/data/transform * Fix calls to `/api/console/proxy` Seen on page: /app/dev_tools#/console * Fix calls to `/api/monitoring/v1/clusters` and `/api/monitoring/v1/elasticsearch_settings/check/cluster` Seen on page: /app/monitoring Co-authored-by: Joe Portner <5295965+jportner@users.noreply.github.com>
* Fix calls to `/api/monitoring/v1/clusters` and `/api/monitoring/v1/elasticsearch_settings/check/cluster` Seen on page: /app/monitoring * Fix calls to `/api/index_management/indices/reload` Seen on page: /app/kibana#/management/elasticsearch/index_management/indices * Fix calls to `/api/cross_cluster_replication/follower_indices` Seen on page: /app/kibana#/management/elasticsearch/cross_cluster_replication/follower_indices * Fix calls to `/api/cross_cluster_replication/auto_follow_patterns` Seen on page: /app/kibana#/management/elasticsearch/cross_cluster_replication/auto_follow_patterns * Fix calls to `/api/remote_clusters` Seen on page: /app/kibana#/management/elasticsearch/remote_clusters/list * Fix calls to `/api/watcher/watches` Seen on page: /app/kibana#/management/elasticsearch/watcher/watches * Fix calls to `/api/rollup/jobs` Seen on page: /app/kibana#/management/elasticsearch/rollup_jobs/job_list
Resolves #91014.
Kibana client-side HTTP requests that happen in an automated/polling fashion need use the
kbn-system-request
header so that the request does not extend authenticated users' sessions.Each commit fixes one or more API calls for a different page. See commit messages for details.