-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[APM] Renames significant terms feature to "Correlations" (#88974) #89028
[APM] Renames significant terms feature to "Correlations" (#88974) #89028
Conversation
Pinging @elastic/apm-ui (Team:apm) |
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.
Had a couple notes but looks good.
@@ -128,7 +128,7 @@ export function LatencyCorrelations() { | |||
<EuiFormRow | |||
fullWidth={true} | |||
label="Field" | |||
helpText="Fields to analyse for significant terms" | |||
helpText="Fields to analyse for correlations" |
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.
We didn't put in any i18n when doing the PoC. Should we start adding it here or open an issue to make sure it gets there for GA?
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'll update #86477 to include the i18n.
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 @ogupte. That's an oversight on my part when I did the initial POC.
category: ['observability'], | ||
name: i18n.translate('xpack.apm.enableCorrelationsExperimentName', { | ||
defaultMessage: 'APM Significant terms (Platinum required)', | ||
defaultMessage: 'APM Correlations (Platinum required)', |
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 don't think this should be capitalized.
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 retained the same capitalization pattern as before, but i agree it doesn't need to be capitalized here
}), | ||
value: false, | ||
description: i18n.translate( | ||
'xpack.apm.enableCorrelationsExperimentDescription', | ||
{ | ||
defaultMessage: | ||
'Enable the experimental Significant terms feature in APM', | ||
defaultMessage: 'Enable the experimental Correlations feature in APM', |
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 don't think this should be capitalized.
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.
Not a native speaker but shouldn't it be capitalised if it's a feature name? It's not just "a correlation". It's the Correlation feature.
Either way, not a big deal :)
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
) (elastic#89028) * [APM] Renames significant terms feature to "Correlations" (elastic#88974) * fix capitalizations Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
) (elastic#89028) * [APM] Renames significant terms feature to "Correlations" (elastic#88974) * fix capitalizations Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Closes #88974. Changes the labels and ui settings key to reflect the actual feature name "Correlations".
This PR does not update names of files, react components or props. These can be addressed later.