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

Api reference docs for state_containers and state_sync #67354

Merged
merged 10 commits into from
Jun 26, 2020

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented May 26, 2020

Summary

  • Adds state_containers and state_sync to api_extractor
  • improves TSDoc definitions for those plugins
  • adds changes to api_extractor script to support common/ folder and runs docs generation sequentially to not get OOM.

Review

Dev Docs

Api reference for state_sync and state_containers available here:


if (opts.help) {
process.stdout.write(
dedent(chalk`
{dim usage:} node scripts/check_published_api_changes [...options]

Checks for any changes to the Kibana Core API
Copy link
Contributor Author

@Dosant Dosant Jun 18, 2020

Choose a reason for hiding this comment

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

I removed Core mentions from API Extractor, because we now use it for plugins also.

return 'public';
case folder.includes('server'):
return 'server';
case folder.includes('common'):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

state_containers are used on server also. so them are in common and we need API Extractor to support common

);
const results = [];
for (const folder of filteredFolders) {
results.push(await run(folder, { log, opts }));
Copy link
Contributor Author

@Dosant Dosant Jun 18, 2020

Choose a reason for hiding this comment

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

instead of running all in parallel now it runs seq.
it is longer, but not crushes with OOM. (with plugin number growth, it would outgrow 10-16-32gb pretty fast)
maybe should be improved to batch multiple in parallel.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be a bit more work, but since this is CPU and memory heavy and we're adding more and more plugins, maybe you could execute a child process for each folder? https://nodejs.org/api/child_process.html#child_process_child_process_exec_command_options_callback

Copy link
Contributor

Choose a reason for hiding this comment

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

I played around with node worker threads in #69615 Although it reduces the memory consumption (no need for extra heap) the speed up isn't enormous (1m50 vs 3m04s) so I think it's fine to leave this synchronous until we have Node12.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's awesome!

export type EnsurePureSelector<T> = Ensure<T, PureSelector<any, any, any>>;
/**
* @public
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am pretty happy with generated docs for state_sync, but for state_containers it is not that great... mostly due how complicate type system of state_containers is.. There are a lot of helper types which are exposed publicly and I am not sure if we should try to document them. don't see much usefulness for documenting all of that and is not a quick effort..

@streamich let me know if you have ideas or suggestions.

@Dosant Dosant changed the title [draft] kibana_utils docs trying api extractor Api reference docs for state_containers and state_sync Jun 18, 2020
@Dosant Dosant added Feature:StateManagement release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:AppArch v7.9.0 v8.0.0 labels Jun 18, 2020
@Dosant Dosant marked this pull request as ready for review June 18, 2020 13:10
@Dosant Dosant requested review from a team as code owners June 18, 2020 13:10
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@Dosant Dosant requested a review from streamich June 18, 2020 13:10
Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

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

+1 For platform changes

@Dosant
Copy link
Contributor Author

Dosant commented Jun 25, 2020

@elasticmachine merge upstream

Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

LGTM

@Dosant
Copy link
Contributor Author

Dosant commented Jun 26, 2020

@streamich, thanks for suggestions!

@Dosant Dosant merged commit b3b5dab into elastic:master Jun 26, 2020
Dosant added a commit to Dosant/kibana that referenced this pull request Jun 26, 2020
Adds state_containers and state_sync to api_extractor
improves TSDoc definitions for those plugins
adds changes to api_extractor script to support common/ folder and runs docs generation sequentially to not get OOM.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Dosant added a commit that referenced this pull request Jun 26, 2020
Adds state_containers and state_sync to api_extractor
improves TSDoc definitions for those plugins
adds changes to api_extractor script to support common/ folder and runs docs generation sequentially to not get OOM.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 26, 2020
* master: (34 commits)
  Upgrade `elliptic` dependency (`6.5.2` → `6.5.3`). (elastic#70054)
  [License Management] Do not break when `telemetry.enabled:false` (elastic#69711)
  [SECURITY] Redirect app/security to app/security/overview (elastic#70005)
  "Explore underlying data" in-chart action (elastic#69494)
  Api reference docs for state_containers and state_sync (elastic#67354)
  prep state transfer for passing embeddables by value to editor and back (elastic#69991)
  move Metrics API to start (elastic#69787)
  refactor: 💡 fix typo in embeddable (elastic#69417)
  [alerting] migrates the old `alerting` consumer to be `alerts` (elastic#69982)
  [APM]Create API to return data to be used on the Overview page (elastic#69137)
  [Lens] Fix delete button position in dimension panel for long labels (elastic#69495)
  [Lens] Add toolbar api (elastic#69263)
  Fixes bug on color picker defaults on TSVB (elastic#69889)
  [DOCS] Fixes wording in Upload a CSV section (elastic#69969)
  [Discover] Validate timerange before submitting query to ES (elastic#69363)
  [Maps] avoid using MAP_SAVED_OBJECT_TYPE constant when defining URL paths (elastic#69723)
  [Maps] Fix icon palettes are not working (elastic#69937)
  [Ingest Manager] Fix typo in constant name (elastic#69919)
  [test] skip status.allowAnonymous tests on cloud (elastic#69017)
  Fix backport (elastic#70003)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:StateManagement release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants