Skip to content
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

[event log] Remove event log HTTP APIs if no longer used #90486

Closed
pmuellr opened this issue Feb 5, 2021 · 8 comments · Fixed by #155913
Closed

[event log] Remove event log HTTP APIs if no longer used #90486

pmuellr opened this issue Feb 5, 2021 · 8 comments · Fixed by #155913
Assignees
Labels
estimate:small Small Estimated Level of Effort Feature:EventLog Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) technical debt Improvement of the software architecture and operational architecture

Comments

@pmuellr
Copy link
Member

pmuellr commented Feb 5, 2021

While reviewing PR #89681, I happened to notice that are URLs are not structured that great.

const BASE_EVENT_LOG_API_PATH = "/api/event_log"

router.get(
{
path: `${BASE_EVENT_LOG_API_PATH}/{type}/{id}/_find`,

router.post(
{
path: `${BASE_EVENT_LOG_API_PATH}/{type}/_find`,

(new API in PR, shaped the same as others)

router.post(
{
path: `${BASE_EVENT_LOG_API_PATH}/{type}/saved_object_summary`,

Two problems:

  • we've now made the path segment after /api/event_log always have to be a type value - since it could be anything, we can't ever create a fixed path like /api/event_log/_do_something, since _do_something could be a SO type
  • we may want to have APIs that work over multiple types, but now we can't, since you have to always have a single type as the path segment - I guess you could pass one in on the path, and others via query string, but that's awkward

Seems like we need to reshape these to allow for more URL patterns in the event log in the future.

@pmuellr pmuellr added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:EventLog labels Feb 5, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

@pmuellr
Copy link
Member Author

pmuellr commented Feb 5, 2021

Here's the kinda shape I'm thinking about, which would be a little more future-friendly. The uri names are just suggestions, we can probably find better ones.

Method Current URI Renamed URI?
GET {type}/{id}/_find _find_by_id/{type}/{id}
POST {type}/_find _find_by_ids/{type}
POST {type}/saved_object_summary _aggs/{type}

@gmmorris gmmorris added the loe:medium Medium Level of Effort label Jul 6, 2021
@gmmorris gmmorris added technical debt Improvement of the software architecture and operational architecture estimate:small Small Estimated Level of Effort labels Aug 13, 2021
@gmmorris gmmorris removed the loe:medium Medium Level of Effort label Sep 2, 2021
@kobelb kobelb added the needs-team Issues missing a team label label Jan 31, 2022
@botelastic botelastic bot removed the needs-team Issues missing a team label label Jan 31, 2022
@mikecote
Copy link
Contributor

mikecote commented Aug 4, 2022

Could also explore removing these APIs if they aren't used.

@pmuellr
Copy link
Member Author

pmuellr commented Nov 3, 2022

It's not clear we even NEED this API anymore. Does anyone use it???

@EricDavisX
Copy link
Contributor

@dolaru are we using it in the new perf test framework?

@pmuellr
Copy link
Member Author

pmuellr commented Mar 2, 2023

As near as I can tell, these APIs are not used by Kibana at runtime, but are used by function tests. So, seems like we should move them INTO the function tests, so we can verify we're not using them, so we don't have to really worry about them anymore. Perhaps when we move them, we can see if we can just convert the uses into direct ES calls, and remove the routes entirely.

Also, since the issue was created, the endpoints have moved to /internal/event_log, so I don't think we have any "breaking" API here.

Not seeing any urgency to get this done, so seems deferrable, but also seems fairly simple.

@pmuellr
Copy link
Member Author

pmuellr commented Mar 2, 2023

Mike pointed out that the Kibana load testing tools we use may be using these APIs as well. We should find out, and then see if those routes (if used) can be replaced with searches directly against the event log indices.

@mikecote mikecote changed the title [event log] restructure route paths to avoid path parameters at "beginning" of the urls [event log] Remove event log HTTP APIs if no longer used Mar 6, 2023
@mikecote
Copy link
Contributor

mikecote commented Mar 6, 2023

I changed the title to [event log] Remove event log HTTP APIs if no longer used. Let's remove them if they aren't used. It feels like they aren't after searching for usage myself.

@doakalexi doakalexi self-assigned this Apr 25, 2023
doakalexi added a commit that referenced this issue May 2, 2023
#155913)

Resolves #90486

## Summary

Removes the event log HTTP apis since they are not used, and adds them
to the functional tests


### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
estimate:small Small Estimated Level of Effort Feature:EventLog Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) technical debt Improvement of the software architecture and operational architecture
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

7 participants