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

[Fleet] Honor registry datastream source_mode in package policy editor UI #147034

Merged
merged 8 commits into from
Dec 7, 2022

Conversation

nchaulet
Copy link
Member

@nchaulet nchaulet commented Dec 5, 2022

Description

Resolve #141211

Honor registry datastream source_mode in package policy editor UI, package can define a source_mode for a package in the UI,
if source_mode is:

  • default we should not allow user to enable the experimental feature
  • synthetic user can disable synthetic source
  • undefined user can enable/disable synthetic source

I did some refacto against the package policy datastream type, and found we persisted the release field.

I also move the experimental datastream to his own component to be able to easily unit test it.

UI Changes

With source_mode default the field is disabled
Screen Shot 2022-12-05 at 1 22 04 PM

With source_mode synthetic
Screen Shot 2022-12-05 at 9 10 16 PM

@nchaulet nchaulet self-assigned this Dec 5, 2022
@nchaulet nchaulet force-pushed the feature-ui-source-mode branch from b1c506c to 5e9aa60 Compare December 5, 2022 22:19
@nchaulet
Copy link
Member Author

nchaulet commented Dec 6, 2022

@elasticmachine merge upstream

@nchaulet nchaulet added release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v8.7.0 labels Dec 6, 2022
@nchaulet nchaulet marked this pull request as ready for review December 6, 2022 02:09
@nchaulet nchaulet requested review from a team as code owners December 6, 2022 02:09
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

Copy link
Contributor

@juliaElastic juliaElastic left a comment

Choose a reason for hiding this comment

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

LGTM, not tested locally.

@ruflin
Copy link
Contributor

ruflin commented Dec 6, 2022

Don't see any docs around this. User will have quite a few questions on the exact behaviour on what the effect of the toggles have. Lets make sure we have a docs page where we can go into the details and also reference when questions come up.

@nchaulet
Copy link
Member Author

nchaulet commented Dec 6, 2022

Don't see any docs around this. User will have quite a few questions on the exact behaviour on what the effect of the toggles have. Lets make sure we have a docs page where we can go into the details and also reference when questions come up.

I am wondering if we could solve this with a tool tip and a in product doc here, instead of adding a new doc?

@nchaulet nchaulet requested a review from shahzad31 December 6, 2022 13:37
@ruflin
Copy link
Contributor

ruflin commented Dec 6, 2022

I am wondering if we could solve this with a tool tip and a in product doc here, instead of adding a new doc?

I suspect the topic is too complex to cover in a single tooltip but if we can, great! This doesn't mean we shouldn't have a tool tip with a short explanation and a link to longer docs / details.

One thing to keep in mind around docs: We can't link anyone to docs only in the UI directly.

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

I'm getting a little confused because this deals with synthetic source which in reality has nothing to do with synthetics, but I see some of our files changed here. Is this just because some changes were made as part of this PR to remove release key from streams?

@nchaulet
Copy link
Member Author

nchaulet commented Dec 6, 2022

I'm getting a little confused because this deals with synthetic source which in reality has nothing to do with synthetics, but I see some of our files changed here. Is this just because some changes were made as part of this PR to remove release key from streams?

@dominiqueclarke Yes that correct the release field do need to be persisted into package policy stream we can retrieve that information from the registry or the installation it's what updated all you test

@nchaulet
Copy link
Member Author

nchaulet commented Dec 6, 2022

@elasticmachine merge upstream

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

Uptime changes LGTM

Copy link
Contributor

@hop-dev hop-dev left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the unit tests too 👍

@nchaulet nchaulet enabled auto-merge (squash) December 7, 2022 14:31
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
fleet 757 758 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
fleet 896.3KB 896.9KB +608.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
fleet 120.4KB 120.5KB +115.0B
Unknown metric groups

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 59 65 +6
osquery 109 115 +6
securitySolution 445 451 +6
total +20

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 68 74 +6
osquery 110 117 +7
securitySolution 521 527 +6
total +21

History

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

cc @nchaulet

@nchaulet nchaulet merged commit f8fea0f into elastic:main Dec 7, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Dec 7, 2022
@ruflin
Copy link
Contributor

ruflin commented Dec 12, 2022

I did some testing on this and so far I'm not sure if it works as expected. The change I made in system.memory dataset manifest is as following:

elasticsearch:
  source_mode: "synthetic"
  index_mode: "time_series"

When I startup with elastic-package, the package is automatically installed. The good news is when I look at the data stream component template, index.mode: synthetic is set on metrics-system.memory-default. But when I now go the elastic-agent policy with memory enabled and look at the synthetic setting it is disabled:

Screenshot 2022-12-12 at 13 20 35

When I toggle it between enable and disable, the "Save policy" button never becomes available, for other datasets where I have not set this setting it just works. Here is the change I'm testing with: elastic/integrations#4749

There is another use case I wanted to test which I couldn't yet because the above already seems to not work as expected in the UI.

  • Synthetic source is enabled in the package
  • Data starts to ship to it and synthetic source is actively disabled
  • User creates a second policy with the same integration. What does the toggle in the policy show now? It should be disable because that is the setting of the data stream that already exists and must overwrite the package default.
  • Same scenario with a different namespace, what happens now?

@nchaulet
Copy link
Member Author

nchaulet commented Dec 12, 2022

@ruflin thanks for testing this I found a few bug with the experimental datastream settings:

  • we are not preserving the already set experimental features when upgrading a package
  • we were not loading correctly the already set experimental feature when creating a new package policy
  • we were mutating the original package policy object when editing experimental feature, the "save integration" button was disabled because of this as the original package policy and the updated one are not different

I am working on a PR to fix that and add tests. (link to the PR #147368)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fleet] Honor value of source_mode for data streams
8 participants