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
[Cases] Deprecate endpoints #124773
[Cases] Deprecate endpoints #124773
Changes from 12 commits
4100b52
0bdefb6
85d0e0c
15bb9d9
8e4a776
d4d8fe2
d59f1b9
7b6cf4f
84ef3b4
e63e3cf
3bebd6b
eb3c26a
4d293cc
1c81792
c17b0d2
07598ab
bac177c
62e5d01
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.
It looks like the warning header will be returned on every call to this endpoint - but don't you just want the header if they use the
query.includeComments
property?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, good point! I will update accordingly.
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.
@pmuellr I just noticed that we default it to
true
if the user omits it so I don't think we can distinguish if the user provided the parameter or not.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, bummer.
You could change the type to
boolean | undefined
, and then apply the default outside of the validation, but that's clumsy, and changes your typing which could require more undefined checks. Perhaps instead, sincefalse
is the "deprecated" version, you can just check after the validation if it'sfalse
and then add the header? So it wouldn't be marked deprecated if they used the param and it was true.Another possibility would be to see if there's a way to get the parameters in more of a "raw" state, after the validation, from the request. And then put the check there.
I really don't think we want this header used every time the API is used.
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.
@pmuellr Amazing idea about the "raw" state. I pushed the changes you 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.
@lukeelmers @kobelb Cases are gonna use these endpoints until we provide alternatives. Is it ok if we log warnings even when Cases are using them?
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.
We don't wanna use
logger.warn
for deprecations. "warn" is a high logging level that should be used when things are going seriously awry.@elastic/kibana-core: did core end up exposing a logger that we should be using for intermittent deprecation usage, for example when a client is calling a HTTP API in a deprecated manner?
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.
@kobelb Unfortunately we do not have something like this. There are a few other places in Kibana I am aware of (specifically in Core & Security) where we are logging deprecations for routes at a
warn
level.We have other work we want to do in this area to improve the deprecation process for HTTP APIs:
Warning
headers for deprecated http routes #105692But to my knowledge we do not have an issue for what you're specifically mentioning here (that is, a dedicated logger exposed to http routes for logging deprecations specifically).
My main concern would be if we are logging deprecation warnings when users aren't initiating the calls to the deprecated API. I would recommend either adding some logic to try to differentiate between Kibana & non-Kibana requests, or perhaps creating a duplicate route that's just for use by Cases, and simply don't log deprecations from that route.
If you want to attempt to differentiate between Kibana & non-Kibana requests & conditionally log based on that info, there is some prior art in the spaces plugin: https://github.com/elastic/kibana/blob/ec30f2aeeb10fb64b507935e558832d3ef5abfaa/x-pack/plugins/spaces/server/usage_stats/usage_stats_client.ts#L113-L118
This is an imperfect workaround, but will address the majority of situations.
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.
Just dropping by to add another related issue: #112291
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 @lukeelmers! I will go with adding some logic to differentiate between Kibana & non-Kibana requests. We plan to introduce different endpoints as alternatives in the future so I will switch Cases to them when the time comes. 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.
If there's precedent for using the
warn
level for this and we haven't had customers complain, feel free to follow suit @cnasikas.I do agree that we shouldn't be logging these deprecations for internal HTTP requests. One other option we could explore is having
/api
public routes and/internal
internal routes, and not doing the deprecation logging for the internal routes.