-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[EDR Workflows] Align advanced option tool-tips with elastic.co documentation #230269
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
Conversation
| ), | ||
| license: 'platinum', | ||
| }, | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the new option that was added.
...ty/plugins/security_solution/public/management/pages/policy/models/advanced_policy_schema.ts
Outdated
Show resolved
Hide resolved
…agement/pages/policy/models/advanced_policy_schema.ts
…to align-advanced-options
|
Cloud deployment initiated, see credentials at: https://buildkite.com/elastic/kibana-deploy-cloud-from-pr/builds/415 |
ashokaditya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes. I've a few suggestions that apply to all the text changes. I think overall, the messages can be improved/simplified so it is easier to understand. I would suggest that we get someone from the docs team to proof read the copy changes here.
...ty/plugins/security_solution/public/management/pages/policy/models/advanced_policy_schema.ts
Outdated
Show resolved
Hide resolved
...ty/plugins/security_solution/public/management/pages/policy/models/advanced_policy_schema.ts
Outdated
Show resolved
Hide resolved
| @@ -152,7 +156,7 @@ export const AdvancedPolicySchema: AdvancedPolicySchemaType[] = [ | |||
| 'xpack.securitySolution.endpoint.policy.advanced.linux.advanced.logging.file', | |||
| { | |||
| defaultMessage: | |||
| 'A supplied value will override the log level configured for logs that are saved to disk and streamed to Elasticsearch. It is recommended Fleet be used to change this logging in most circumstances. Allowed values are error, warning, info, debug, and trace.', | |||
| "Override the log level configured for logs that are saved to disk and streamed to Elasticsearch. Elastic recommends using Fleet to change this logging setting in most circumstances. Allowed values are 'error', 'warning', 'info', 'debug', and 'trace'. Default: Fleet configuration is used.", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think logging level since we have logging setting later in the sentence?
| "Override the log level configured for logs that are saved to disk and streamed to Elasticsearch. Elastic recommends using Fleet to change this logging setting in most circumstances. Allowed values are 'error', 'warning', 'info', 'debug', and 'trace'. Default: Fleet configuration is used.", | |
| "Override the logging level configured for logs that are saved to disk and streamed to Elasticsearch. Elastic recommends using Fleet to change this logging setting in most circumstances. Allowed values are 'error', 'warning', 'info', 'debug', and 'trace'. Default: Fleet configuration is used.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "log level" is the normal phrase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I would then suggest that we use ...log setting... or just setting instead of ...logging setting.... Same suggestion for all other changes that use similar phrasing.
...ty/plugins/security_solution/public/management/pages/policy/models/advanced_policy_schema.ts
Outdated
Show resolved
Hide resolved
...ty/plugins/security_solution/public/management/pages/policy/models/advanced_policy_schema.ts
Outdated
Show resolved
Hide resolved
...ty/plugins/security_solution/public/management/pages/policy/models/advanced_policy_schema.ts
Outdated
Show resolved
Hide resolved
...ty/plugins/security_solution/public/management/pages/policy/models/advanced_policy_schema.ts
Outdated
Show resolved
Hide resolved
...ty/plugins/security_solution/public/management/pages/policy/models/advanced_policy_schema.ts
Outdated
Show resolved
Hide resolved
| @@ -2176,7 +2205,7 @@ export const AdvancedPolicySchema: AdvancedPolicySchemaType[] = [ | |||
| 'xpack.securitySolution.endpoint.policy.advanced.windows.advanced.events.aggregate_process', | |||
| { | |||
| defaultMessage: | |||
| 'Reduce event volume by merging related process events into fewer aggregate events. <=8.17 default: false, >=8.18 default: true', | |||
| 'Reduce event volume by merging related process events into fewer aggregate events. Default: <=8.17: false, >=8.18: true.', | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compared to previous Default values above, these default values that are specific to a stack version are confusing to read and understand. For <=8.17, Default: true. For >=8.18 Default: false. might be better and easier to understand and that version info does not go into the input field. Same for other values where we have version specific default values.
| 'Reduce event volume by merging related process events into fewer aggregate events. Default: <=8.17: false, >=8.18: true.', | |
| 'Reduce event volume by merging related process events into fewer aggregate events. For <=8.17, Default: false. For >=8.18, Default: true.', |
Why not make it even simpler by saying For 8.17 and below, Default: false. For 8.18 and greater Default: true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@natasha-moore-elastic do you have an opinion here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that this would be easier to read. I'd suggest:
For 8.17 and earlier, default: false. For 8.18 and later, default: true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I updated all options with similar text. e70d2ad
| @@ -1003,7 +1017,7 @@ export const AdvancedPolicySchema: AdvancedPolicySchemaType[] = [ | |||
| 'xpack.securitySolution.endpoint.policy.advanced.linux.advanced.memory_protection.enable_shared_dirty_scan', | |||
| { | |||
| defaultMessage: | |||
| 'Instead of ignoring regions with just no Private_Dirty bytes, ingore regions with the combination of no Private_Dirty bytes, no Shared_Dirty bytes and is file backed. This has the effect of scanning more memory regions because of the loosened restrictions. Default: true.', | |||
| "Instead of ignoring regions with just no 'Private_Dirty' bytes, ignore regions with the combination of no 'Private_Dirty' bytes, no 'Shared_Dirty' bytes and is file-backed. This has the effect of scanning more memory regions because of the loosened restrictions. Default: true.", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this explains how to use the input values correctly. Is ignoring memory regions is achieved by not adding Private_Dirty in the input filed? or no Private_Dirty in the input field. Also I think the message should say Instead of ignoring memory regions....?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the wording to say "...ignoring memory regions...". The input type is a boolean, which is implied by Default: true. as we normally do.
|
Thanks for all your feedback @ashokaditya . @dasansol92 @ashokaditya I let this PR fall dormant in the run up to 9.2 so I didn't cause conflicts for others, but it's ready for review (again) now. Please review the backport label and let me know if it should change. |
…to align-advanced-options
Co-authored-by: Ash <[email protected]>
Sorry @ashokaditya I just noticed this bit. These changes are to align Kibana with the text for this online page, which was proofread by the docs team. So that has happened. There are a lot of changes in this PR already. Once it merges I'll follow up with another, smaller batch of changes -- again to align with the docs-team-approved online documentation. |
| @@ -152,7 +156,7 @@ export const AdvancedPolicySchema: AdvancedPolicySchemaType[] = [ | |||
| 'xpack.securitySolution.endpoint.policy.advanced.linux.advanced.logging.file', | |||
| { | |||
| defaultMessage: | |||
| 'A supplied value will override the log level configured for logs that are saved to disk and streamed to Elasticsearch. It is recommended Fleet be used to change this logging in most circumstances. Allowed values are error, warning, info, debug, and trace.', | |||
| "Override the log level configured for logs that are saved to disk and streamed to Elasticsearch. Elastic recommends using Fleet to change this logging setting in most circumstances. Allowed values are 'error', 'warning', 'info', 'debug', and 'trace'. Default: Fleet configuration is used.", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I would then suggest that we use ...log setting... or just setting instead of ...logging setting.... Same suggestion for all other changes that use similar phrasing.
I don't see any backport labels on the PR yet. Do we want to backport this to |
|
Thanks for review.
I don't think we need to backport this. |
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Async chunks
History
|
…entation (elastic#230269) ## Summary This aligns the tool-tips shown in Kibana to the same Docs-Team-approved-text on https://www.elastic.co/docs/reference/security/defend-advanced-settings (including a few pending updates in elastic/docs-content#2365). One new advanced option is added. It's been missing from Kibana -- it isn't new. ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) ### Identify risks No risk assuming this renders right in Kibana. It's just tool-tip wording changes. --------- Co-authored-by: Ash <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Summary
This aligns the tool-tips shown in Kibana to the same Docs-Team-approved-text on https://www.elastic.co/docs/reference/security/defend-advanced-settings (including a few pending updates in elastic/docs-content#2365). One new advanced option is added. It's been missing from Kibana -- it isn't new.
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
Identify risks
No risk assuming this renders right in Kibana. It's just tool-tip wording changes.