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

[Logs UI] Reimplement log source configuration routes in plain HTTP+JSON #64021

Merged

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented Apr 20, 2020

Summary

This reimplements the parts of the source configuration GraphQL API relevant for logs in HTTP+JSON. The source configuration UI components specific for the Logs UI are moved to the pages/logs/settings directory.

The storage layer is not changed, so the saved object is still shared with the Metrics UI. But this separation on the route level prepares for an eventual split of the source configuration storage mechanisms.

The source status is implemented as separate route to allow for lazy field cap querying (see phase one of #53138).

It leaves the previous GraphQL resolvers in place for use by the Metrics UI. They will be removed along with the whole GraphQL endpoint in a later PR (see phase two of #53138).

New routes

GET /api/infra/log_source_configurations/{sourceId}

Get a log source configuration from the saved objects or the defaults.

PATCH /api/infra/log_source_configurations/{sourceId}

Update a log source configuration's saved object.

GET /api/infra/log_source_configurations/{sourceId}/status

Get the index names and field list pointed to by a source configuration.

Testing

Some points to have an eye on:

  • The UX should not have changed at all except that log source loading might be a bit faster.
  • The Logs UI should not perform any GraphQL requests anymore. They should all be to the /api/infra/log_source_configurations/... endpoints.
  • The "create" route was not implemented separately, but subsumed in the "update" route. Nevertheless it should be possible to store the initial configuration as well as update it later via the settings tab.
  • Changes to the source name and timestamp field should be reflected in the Metrics UI, but other source configuration properties (such as the metrics indices) should be preserved (and not be reset to the default).
  • Source configurations saved before should remain usable.

Follow-up tasks

  • Reimplement a similar API for metrics sources
  • Remove and dependencies of the Metrics UI on GraphQL types
  • Remove the GraphQL infra endpoint
  • Remove the GraphQL type generation script

@weltenwort weltenwort added v8.0.0 Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes v7.8.0 labels Apr 20, 2020
@weltenwort weltenwort self-assigned this Apr 20, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@weltenwort weltenwort marked this pull request as ready for review April 21, 2020 18:50
@weltenwort weltenwort requested a review from a team as a code owner April 21, 2020 18:50
@Kerry350 Kerry350 self-requested a review April 23, 2020 07:51
@weltenwort
Copy link
Member Author

@elasticmachine merge upstream

@weltenwort
Copy link
Member Author

There's a significant collision of intended goals with #64126, which I'll try to resolve.

@weltenwort
Copy link
Member Author

@elasticmachine merge upstream

@Kerry350 Kerry350 mentioned this pull request Apr 23, 2020
Copy link
Contributor

@Kerry350 Kerry350 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks / works great 👍 This will make the alerting stuff much easier 🙏

Just a few small comments.

type ValdidationResult<Value> = ReturnType<RouteValidationFunction<Value>>;

export const createValidationFunction = <A, O, I>(
runtimeType: Type<A, O, I>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit: I personally find generics easier / quicker to parse with meaningful type variables, could we change these from A, O, I?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I absolutely agree. I stuck with the io-ts naming scheme during experimentation and forgot to clean it up afterwards.

@weltenwort
Copy link
Member Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

@weltenwort
Copy link
Member Author

@Kerry350 I had to adapt the mount location of the view-in-context-related component hierarchy to make sure the providers are available. I'd appreciate a second pair of eyes on that to ensure I haven't broken the view-in-context modal if you have the time 🤔

@Kerry350
Copy link
Contributor

@weltenwort

Screenshot 2020-04-27 16 14 20

LGTM 👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@weltenwort weltenwort merged commit 2fba7ed into elastic:master Apr 28, 2020
@weltenwort weltenwort deleted the logs-ui-reimplement-source-routes-as-http branch April 28, 2020 09:12
weltenwort added a commit to weltenwort/kibana that referenced this pull request Apr 28, 2020
weltenwort added a commit that referenced this pull request Apr 28, 2020
…HTTP+JSON (#64021) (#64626)

Backports the following commits to 7.x:
 - [Logs UI] Reimplement log source configuration routes in plain HTTP+JSON (#64021)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Logs UI Logs UI feature release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants