Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[http] Do not do client version check on serverless as we do for onprem #159101
[http] Do not do client version check on serverless as we do for onprem #159101
Changes from 9 commits
450926f
15399e1
da8b1a4
4e722e6
dbcef06
67e6187
8b31fcf
d21ae9a
4e44fbe
09d76f5
52d585c
d827f55
84dfedc
f12482a
4850551
bd0ea15
a67fd82
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 know it was done to avoid having to change all the constructor calls, but I wonder how safe having this
options
be an optional parameter is. I don't think we're using theRouter
in that many places, so maybe makingoptions
mandatory now could make sense, 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.
Sounds good to me!
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Bring this more in line with other ways of configuring Kibana for serverless
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: Handler resolution or version resolution? Is the "oldest handler" really a valid term?
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.
Will change to
versionResolution
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 wonder if we still want to warn via some custom header in the response.
This would help when troubleshooting HARs, or even, potentially using that on the browser to warn the user that they should refresh the page.
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 like that idea! As you suggest, this is one way to drive the "update available" UX we've spoken about... Currently it will only detect a new version when we ship a new stack version --- this will not be sufficient for more continuous updates. I think updating the Kibana version to contain a git hash or build nr or something would be needed...
I'd like to spend a bit more time thinking about how we want to approach that specifically. Do you think it is still worth adding a header like this now? If so, do you think we should just reuse the existing
kbn-version
header and set it toupdate-available
orout-of-sync
?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.
Opened #159127
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.
IMO, we should just include the version in the header all the time, and add metadata to our logging-- not just error logging-- with the client version and server version. That metadata will be invaluable to not only knowing if a mismatch is contributing to an error, but also if an error has spiked on a particular version of client + server, and how many clients are "lagging" the upgrade curve.
The version mismatch will drive the UX, for certain, but I would love to have requests and errors tagged all the time. The mismatch will be one data point in an investigation... an important one, but not the only one.