[APM] Agent configuration GA #46995

sorenlouv · 2019-09-30T21:27:13Z

Fixes #44475
Fixes #43351
Fixes #43354
Fixes #45124

User facing changes:

Add two more settings: "capture body" and "transaction max spans"
- not displayed for RUM services
Show status indicating whether a configuration has been applied by any agents or not
Allow user to create configurations for "All" services and/or "All" environments
Minor UI fixes: loading states
Remove beta label

Non-user facing changes:

Rename "Settings" to "Agent configurations"
Extract TimestampSummaryItem and rename to TimestampTooltip
add forceCache option to callApi to force the use of caching despite caching logic only allows caching for endpoints with date ranges
remove unused TransactionStickyProperties component

elasticmachine · 2019-09-30T21:28:11Z

Pinging @elastic/apm-ui

sorenlouv · 2019-09-30T21:32:22Z

Not too bad for a new feature :D

sorenlouv · 2019-09-30T21:47:46Z

...gins/apm/public/components/app/Settings/AgentConfigurations/AddEditFlyout/ServiceSection.tsx

+const t = (id: string, defaultMessage: string) =>
+  i18n.translate(`xpack.apm.settings.agentConf.flyOut.serviceSection.${id}`, {
+    defaultMessage
+  });


The i18n strings were getting long and starting to be a pain on the eyes. I played around with some different formats and I'm pretty pleased with this. Not saying we should do it everywhere but I think it's worth experimenting with. A couple of goals:

the resulting call t('key', 'label') should be as short and concise as possible

when renaming a file it should be easy to rename all i18n strings simultaneously

the wrapper should not be too abstracted away from the original

I'm assuming this breaks the i18n check?

Maybe we can stop using a hierarchy for our labels. Instead of using xpack.apm.settings.agentConfig.flyout.serviceSection.deleteConfigSucceededTitle, we can use xpack.apm.deleteAgentConfigSucceededTitle?

I'm assuming this breaks the i18n check?

Hmm.. I do get this error when running the i18n check:

✖ x-pack/legacy/plugins/apm → EISDIR: illegal operation on a directory, read

Any idea how to fix?

Maybe we can stop using a hierarchy for our labels. Instead of using xpack.apm.settings.agentConfig.flyout.serviceSection.deleteConfigSucceededTitle, we can use xpack.apm.deleteAgentConfigSucceededTitle

That would still be something like:

i18n.translate('xpack.apm.deleteAgentConfigSucceededTitle', { defaultMessage: 'Delete configuration' }),

vs

t('deleteAgentConfigSucceededTitle', 'Delete configuration'),

Ofc if we can't make the later work then there's no way around it, but after having played around with it, it's a joy to have strings as one-liners again, and my eyes slowly start reading the code again (instead of just ignoring all the stuff my brain perceives as noise)

sorenlouv · 2019-09-30T22:20:58Z

...egacy/plugins/apm/public/components/app/Settings/AgentConfigurations/AddEditFlyout/index.tsx

+                  value: transactionMaxSpans,
+                  set: setTransactionMaxSpans,
+                  isValid: isTransactionMaxSpansValid
+                }}


I'm not sure it is the best approach to group the values like this. I could also drop the grouping:

<SettingsSection // sample rate sampleRate={sampleRate} setSampleRate={setSampleRate} isSampleRateValid={isSampleRateValid} // capture body captureBody={captureBody} setCaptureBody={setCaptureBody} // transaction max spans transactionMaxSpans={transactionMaxSpans} setTransactionMaxSpans={setTransactionMaxSpans} isTransactionMaxSpansValid={isTransactionMaxSpansValid} />;

sorenlouv · 2019-09-30T22:22:21Z

...egacy/plugins/apm/public/components/app/Settings/AgentConfigurations/AddEditFlyout/index.tsx

+
+  try {
+    const configuration = {
+      agent_name: 'TODO',


Note to self: fix

elasticmachine · 2019-09-30T22:44:43Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-10-01T12:08:24Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

dgieselaar · 2019-10-01T12:19:34Z

x-pack/legacy/plugins/apm/common/runtime_types/transaction_sample_rate_rt/index.ts

@@ -10,8 +10,8 @@ export const transactionSampleRateRt = new t.Type<number, number, unknown>(
  'TransactionSampleRate',
  t.number.is,
  (input, context) => {
-    const value = Number(input);
-    return value >= 0 && value <= 1 && Number(value.toFixed(3)) === value
+    const value = parseFloat(input as string);


just curious, why parseFloat? because it's more permissive than Number?

The opposite actually. Due to coercion Number('') => 0 whereas parseFloat('') => NaN.
I wanted to disallow empty values.

dgieselaar · 2019-10-01T12:24:44Z

...gins/apm/public/components/app/Settings/AgentConfigurations/AddEditFlyout/ServiceSection.tsx

+const t = (id: string, defaultMessage: string) =>
+  i18n.translate(`xpack.apm.settings.agentConf.flyOut.serviceSection.${id}`, {
+    defaultMessage
+  });


I'm assuming this breaks the i18n check?

dgieselaar · 2019-10-01T12:30:19Z

The service name button alignment looks off (not sure if it's fixable)

dgieselaar · 2019-10-01T13:15:56Z

Ah, makes sense. parseFloat however will also get numbers out of `1x` etc though. But maybe less of an issue here.

…

On Tue, Oct 1, 2019 at 2:53 PM Søren Louv-Jansen ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In x-pack/legacy/plugins/apm/common/runtime_types/transaction_sample_rate_rt/index.ts <#46995 (comment)>: > @@ -10,8 +10,8 @@ export const transactionSampleRateRt = new t.Type<number, number, unknown>( 'TransactionSampleRate', t.number.is, (input, context) => { - const value = Number(input); - return value >= 0 && value <= 1 && Number(value.toFixed(3)) === value + const value = parseFloat(input as string); The opposite actually. Due to coercion Number('') => 0 whereas parseFloat('') => NaN. I wanted to disallow empty values. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#46995?email_source=notifications&email_token=AACWDXHTNQYA2KLUOGDY5V3QMNB43A5CNFSM4I4A5J6KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCGPCUVY#discussion_r330038778>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACWDXELICPPWZ5GNFCXZLDQMNB43ANCNFSM4I4A5J6A> .

dgieselaar · 2019-10-01T13:17:41Z

I'm not sure if that error is related, but the i18n check can probably no longer resolve the message ids if you use an intermediate function. I'd guess the only way to solve it is to take it to the translations or ops team, whoever owns the message extraction process. I do agree that the current syntax is not the most concise.

…

On Tue, Oct 1, 2019 at 3:08 PM Søren Louv-Jansen ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In x-pack/legacy/plugins/apm/public/components/app/Settings/AgentConfigurations/AddEditFlyout/ServiceSection.tsx <#46995 (comment)>: > + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { EuiTitle, EuiSpacer, EuiFormRow } from ***@***.***/eui'; +import React from 'react'; +import { i18n } from ***@***.***/i18n'; +import { SelectWithPlaceholder } from '../../../../shared/SelectWithPlaceholder'; +import { useFetcher } from '../../../../../hooks/useFetcher'; +import { ENVIRONMENT_NOT_DEFINED } from '../../../../../../common/environment_filter_values'; +import { Config } from '../index'; +import { callApmApi } from '../../../../../services/rest/callApmApi'; +const t = (id: string, defaultMessage: string) => + i18n.translate(`xpack.apm.settings.agentConf.flyOut.serviceSection.${id}`, { + defaultMessage + }); I'm assuming this breaks the i18n check? Hmm.. I do get this error when running the i18n check: ✖ x-pack/legacy/plugins/apm → EISDIR: illegal operation on a directory, read Any idea how to fix? Maybe we can stop using a hierarchy for our labels. Instead of using xpack.apm.settings.agentConfig.flyout.serviceSection.deleteConfigSucceededTitle, we can use xpack.apm.deleteAgentConfigSucceededTitle That would still be something like: i18n.translate('xpack.apm.deleteAgentConfigSucceededTitle', { defaultMessage: 'Delete configuration' }), vs t('deleteAgentConfigSucceededTitle', 'Delete configuration'), Ofc if we can't make the later work then there's no way around it, but after having played around with it, it's a joy to have strings as one-liners again, and it my eyes slowly start reading the code again (instead of just ignoring all the stuff my brain perceives as noise) — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#46995?email_source=notifications&email_token=AACWDXF2XH225EJUSQXWKVLQMNDV5A5CNFSM4I4A5J6KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCGPE67Q#discussion_r330045981>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACWDXARXBFBRO6A3SYIBJTQMNDV5ANCNFSM4I4A5J6A> .

sorenlouv · 2019-10-01T13:19:39Z

The service name button alignment looks off (not sure if it's fixable)

Good catch. Turns out there is a way to "flush" a buttons margins:
https://elastic.github.io/eui/#/navigation/button (Flush ButtonEmpty)

dgieselaar · 2019-10-01T13:20:32Z

Sorry for emailing back, my reply boxes have gone missing 😅

dgieselaar

@sqren do you think it's worth to see what it would look like when we use io-ts for the validation of the entire form, rather than stitching stuff together? Would be happy to take a stab at it.

...ugins/apm/public/components/app/Settings/AgentConfigurations/AddEditFlyout/DeleteSection.tsx

dgieselaar · 2019-10-01T12:59:55Z

x-pack/legacy/plugins/apm/server/lib/settings/agent_configuration/search.ts

@@ -48,5 +44,7 @@ export async function searchConfigurations({
  };

  const resp = await client.search<AgentConfiguration>(params);
-  return resp.hits.hits[0];
+
+  type FirstHit = SearchResponse<AgentConfiguration>['hits']['hits'][0];


What's the value of this over just using AgentConfiguration directly?

I wanted to make sure the method returned an optional (x | undefined). The full ES document is returned not just AgentConfiguration. I was looking for something like ESSearchHit<AgentConfiguration> but didn't find anything.

Oh, I get it now. Maybe we can expose something like that as well in our ES typings.

Good idea. I can add it. ESSearchHit good with you or any suggestions?

dgieselaar · 2019-10-01T13:07:27Z

x-pack/legacy/plugins/apm/server/lib/settings/agent_configuration/get_environments/index.ts

  ]);

  return allEnvironments.map(environment => {
    return {
      name: environment,
-      available: !unavailableEnvironments.includes(environment)
+      alreadyExists: existingEnvironments.includes(environment)


Hmm, I guess this is kind of where "existingEnvironments" breaks down. Maybe unconfigured or available is better here.

I found available to be misleading so changed it. But perhaps it's not objectively a good change.
I think unconfigured works 👍

x-pack/legacy/plugins/apm/server/routes/settings.ts

dgieselaar · 2019-10-01T13:19:23Z

x-pack/legacy/plugins/apm/server/routes/settings.ts

-    })
-  ])
+    t.type({ name: t.string }),
+    t.partial({ environments: t.array(t.string) })


should this not be environment? I might have mixed up environment from the configuration and (the available) environments from the UI.

Yes, a configuration is only linked to a single environment.

Oh sorry! I thought I changed this to environment already so when I saw your comment I misunderstood. Must have undone my changes. Will make it environment (and not environments)

dgieselaar · 2019-10-01T14:15:07Z

...gins/apm/public/components/app/Settings/AgentConfigurations/AddEditFlyout/ServiceSection.tsx

+}: Props) {
+  const { data: serviceNames = [], status: serviceNamesStatus } = useFetcher(
+    () => {
+      return callApmApi({


considering that this data could change, should we even cache it?

Hmm... I thought about it but I think it's okay since it's only in-memory caching, so whenever the user comes back to Kibana they will see fresh data. It is also not data that changes very often. Most users have long running application names. Do you think it's likely that someone changes the service name or adds a new service while being on this page?

Hmm, maybe. I can imagine they want to configure a service around the time it starts sending data. Then again, they will probably just refresh the page anyway.

sorenlouv · 2019-10-01T15:38:18Z

do you think it's worth to see what it would look like when we use io-ts for the validation of the entire form, rather than stitching stuff together? Would be happy to take a stab at it. @dgieselaar

Definitely, would like to see that.

dgieselaar · 2019-10-03T06:05:10Z

Here's what it looks like with fully shared validation sorenlouv#15

elasticmachine · 2019-10-03T12:04:08Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: cfe69f2

elasticmachine · 2019-10-09T08:22:23Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 26402e5

dgieselaar

LGTM, just a few nits about translations etc.

...gins/apm/public/components/app/Settings/AgentConfigurations/AddEditFlyout/ServiceSection.tsx

...ins/apm/public/components/app/Settings/AgentConfigurations/AddEditFlyout/SettingsSection.tsx

...r/lib/settings/agent_configuration/get_environments/get_existing_environments_for_service.ts

x-pack/legacy/plugins/apm/server/lib/settings/agent_configuration/get_service_names.ts

x-pack/legacy/plugins/apm/server/routes/create_apm_api.ts

x-pack/legacy/plugins/apm/server/lib/settings/agent_configuration/search.ts

x-pack/test/api_integration/apis/apm/agent_configuration.ts

x-pack/test/api_integration/apis/apm/index.ts

x-pack/test/api_integration/apis/apm/agent_configuration.ts

elasticmachine · 2019-10-09T21:31:35Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 3d60b96

elasticmachine · 2019-10-09T22:51:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 021c174

elasticmachine · 2019-10-10T08:11:08Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: cb9183f

dgieselaar

LGTM! Should we create an issue for sorting in ES, or are we giving up on that requirement?

… Agent configuration phase 2 (#46995) (#47806) * [APM] Use new platform for toast notifications (#47276) * [APM] Use new platform for toast notifications * fix more tests * remove comment * [APM] Agent configuration phase 2 (#46995) * [APM] Agent Config Management Phase 2 * Add status indicator * Extract TimestampTooltip component * Remove unused StickyTransactionProperties component * Fix snapshot and minor cleanup * Minor cleanup * Display settings conditionally by agent name * Fix client * Format timestamp * Minor design feedback * Clear cache when clicking refresh * Fix test * Revert t() short hand * Fix translations * Add support for “all” option * Fix API tests * Move delete button to footer * Fix snapshots * Add API tests * Fix toasts * Address feedback and ensure order when searching for configs * Fix snapshots * Remove timeout

elasticmachine · 2019-10-10T13:54:47Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: cb9183f

sorenlouv requested a review from a team as a code owner September 30, 2019 21:27

sorenlouv added release_note:enhancement Team:APM All issues that need APM UI Team support v7.5.0 labels Sep 30, 2019

sorenlouv requested review from ogupte, dgieselaar and smith September 30, 2019 21:29

sorenlouv force-pushed the apm-acm-extended-options branch from fcf864a to af55e78 Compare September 30, 2019 21:33

sorenlouv commented Sep 30, 2019

View reviewed changes

sorenlouv force-pushed the apm-acm-extended-options branch from 826e474 to 72845b4 Compare October 1, 2019 10:56

dgieselaar reviewed Oct 1, 2019

View reviewed changes

sorenlouv added the [zube]: In Progress label Oct 1, 2019

dgieselaar reviewed Oct 1, 2019

View reviewed changes

sorenlouv self-assigned this Oct 2, 2019

sorenlouv removed the Team:APM All issues that need APM UI Team support label Oct 2, 2019

jalvz mentioned this pull request Oct 3, 2019

agent config GA elastic/apm-server#2747

Merged

sorenlouv added 6 commits October 9, 2019 01:53

Add support for “all” option

f7aac55

Fix API tests

7410fb3

Move delete button to footer

35c734b

Fix snapshots

2e5dc14

Add API tests

7dec317

Fix toasts

26402e5

sorenlouv force-pushed the apm-acm-extended-options branch from 0bf0dc8 to 26402e5 Compare October 9, 2019 07:15

dgieselaar approved these changes Oct 9, 2019

View reviewed changes

Address feedback and ensure order when searching for configs

3d60b96

Fix snapshots

021c174

Remove timeout

cb9183f

dgieselaar approved these changes Oct 10, 2019

View reviewed changes

sorenlouv merged commit 79fb749 into elastic:master Oct 10, 2019

sorenlouv deleted the apm-acm-extended-options branch October 10, 2019 10:16

sorenlouv mentioned this pull request Oct 10, 2019

[7.x] [APM] Use new platform for toast notifications (#47276) | [APM] Agent configuration phase 2 (#46995) #47806

Merged

sorenlouv changed the title ~~[APM] Agent configuration phase 2~~ [APM] Agent configuration GA Oct 10, 2019

sorenlouv added the apm-test-plan-7.5.0 label Oct 10, 2019

sorenlouv removed their assignment Oct 21, 2019

ogupte self-assigned this Oct 21, 2019

ogupte added the apm:test-plan-done Pull request that was successfully tested during the test plan label Oct 22, 2019

[APM] Agent configuration GA #46995

[APM] Agent configuration GA #46995

Conversation

sorenlouv commented Sep 30, 2019 • edited Loading

elasticmachine commented Sep 30, 2019

sorenlouv commented Sep 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Sep 30, 2019

💔 Build Failed

elasticmachine commented Oct 1, 2019

💔 Build Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgieselaar commented Oct 1, 2019

dgieselaar commented Oct 1, 2019 via email

dgieselaar commented Oct 1, 2019 via email

sorenlouv commented Oct 1, 2019

dgieselaar commented Oct 1, 2019

dgieselaar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv commented Oct 1, 2019

dgieselaar commented Oct 3, 2019

elasticmachine commented Oct 3, 2019

💔 Build Failed

elasticmachine commented Oct 9, 2019

💚 Build Succeeded

dgieselaar left a comment

Choose a reason for hiding this comment

elasticmachine commented Oct 9, 2019

💔 Build Failed

elasticmachine commented Oct 9, 2019

💚 Build Succeeded

elasticmachine commented Oct 10, 2019

💚 Build Succeeded

dgieselaar left a comment

Choose a reason for hiding this comment

elasticmachine commented Oct 10, 2019

💚 Build Succeeded

sorenlouv commented Sep 30, 2019 •

edited

Loading

sorenlouv Oct 1, 2019 •

edited

Loading

sorenlouv Oct 1, 2019 •

edited

Loading

sorenlouv Oct 1, 2019 •

edited

Loading