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] [skip-ci] Demonstrate blockers for writing custom search strategies effectively #67533

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented May 27, 2020

⚠️ This is just for demonstration purposes and should never be merged!

Summary

This tries to demonstrate some aspects of the data plugin's search strategy API that keep me from writing solution-specific search strategies:

  • The search strategy context can only be extended via declaration merging, which incorrectly leaks the plugin-specific context provider names to unrelated call sites.
  • There is no way to compose search strategies. In particular, it's not possible to wrap an existing search strategy with a custom one, because the data plugin is not available inside of a search strategy handler.

These points are further illustrated by inline comments below.

@weltenwort weltenwort added Feature:Logs UI Logs UI feature R&D Research and development ticket (not meant to produce code, but to make a decision) Team:infra-logs-ui labels May 27, 2020
@weltenwort weltenwort self-assigned this May 27, 2020
// Custom search strategies
plugins.data.search.registerSearchStrategyContext(
this.initializerContext.opaqueId,
'logSources',
Copy link
Member Author

Choose a reason for hiding this comment

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

The compiler rejects this unless the ISearchContext has been extended using declaration merging. But then it incorrectly leaks this key to other callers.

Comment on lines +22 to +35
// something using data.search.search and payload
const { firstTotal, firstLoaded, firstResults } = await somethingThatCallsAnotherSearchStrategy(
payload
);

// something using data.search.search and the first results
const {
secondTotal,
secondLoaded,
secondResults,
} = await somethingThatCallsAnotherSearchStrategy(firstResults);

const total = mergeTotalsSomehow(firstTotal, secondTotal);
const loaded = mergeLoadedSomehow(firstLoaded, secondLoaded);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is meant to symbolize a sequential composition of two calls to other search strategies. Ideally parallel composition would also be possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Supporting streaming results in the general case will also be tricky.

Copy link
Member Author

@weltenwort weltenwort May 28, 2020

Choose a reason for hiding this comment

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

After some more thought providing combinators for the general case is probably not feasible. Maybe thoroughly documenting the runtime semantics and response shape would be sufficient to enable devs to create specific composed strategies.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kibanamachine
Copy link
Contributor

💔 Build Failed

Failed CI Steps

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

@weltenwort weltenwort changed the title [Logs UI] Demonstrate blockers for writing custom search strategies effectively [Logs UI] [skip-ci] Demonstrate blockers for writing custom search strategies effectively May 27, 2020
@weltenwort weltenwort added Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services and removed Team:infra-logs-ui labels May 28, 2020
@elasticmachine
Copy link
Contributor

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

@lizozom lizozom mentioned this pull request May 31, 2020
32 tasks
@weltenwort
Copy link
Member Author

The data plugin has received an updated API, so this doesn't apply anymore. Thanks, @lizozom, for accommodating the requests!

@weltenwort weltenwort closed this Aug 24, 2020
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 R&D Research and development ticket (not meant to produce code, but to make a decision) Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants