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

[APM] Agent configuration GA #46995

Merged
merged 23 commits into from
Oct 10, 2019
Merged

Conversation

sorenlouv
Copy link
Member

@sorenlouv sorenlouv commented Sep 30, 2019

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

@sorenlouv sorenlouv requested a review from a team as a code owner September 30, 2019 21:27
@sorenlouv sorenlouv added release_note:enhancement Team:APM All issues that need APM UI Team support v7.5.0 labels Sep 30, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui

@sorenlouv
Copy link
Member Author

Not too bad for a new feature :D
Screen Shot 2019-09-30 at 23 31 37

@sorenlouv sorenlouv force-pushed the apm-acm-extended-options branch from fcf864a to af55e78 Compare September 30, 2019 21:33
const t = (id: string, defaultMessage: string) =>
i18n.translate(`xpack.apm.settings.agentConf.flyOut.serviceSection.${id}`, {
defaultMessage
});
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming this breaks the i18n check?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@sorenlouv sorenlouv Oct 1, 2019

Choose a reason for hiding this comment

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

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)

value: transactionMaxSpans,
set: setTransactionMaxSpans,
isValid: isTransactionMaxSpansValid
}}
Copy link
Member Author

Choose a reason for hiding this comment

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

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}
/>;


try {
const configuration = {
agent_name: 'TODO',
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to self: fix

@elasticmachine
Copy link
Contributor

💔 Build Failed

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

💔 Build Failed

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

I'm assuming this breaks the i18n check?

@dgieselaar
Copy link
Member

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

@dgieselaar
Copy link
Member

dgieselaar commented Oct 1, 2019 via email

@dgieselaar
Copy link
Member

dgieselaar commented Oct 1, 2019 via email

@sorenlouv
Copy link
Member Author

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
Copy link
Member

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

Copy link
Member

@dgieselaar dgieselaar left a comment

Choose a reason for hiding this comment

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

@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.

@@ -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];
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@sorenlouv sorenlouv Oct 1, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

]);

return allEnvironments.map(environment => {
return {
name: environment,
available: !unavailableEnvironments.includes(environment)
alreadyExists: existingEnvironments.includes(environment)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

})
])
t.type({ name: t.string }),
t.partial({ environments: t.array(t.string) })
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@sorenlouv sorenlouv Oct 1, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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.

@sorenlouv sorenlouv self-assigned this Oct 2, 2019
@sorenlouv sorenlouv removed the Team:APM All issues that need APM UI Team support label Oct 2, 2019
@dgieselaar
Copy link
Member

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

@elasticmachine
Copy link
Contributor

💔 Build Failed

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

💚 Build Succeeded

Copy link
Member

@dgieselaar dgieselaar left a comment

Choose a reason for hiding this comment

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

LGTM, just a few nits about translations etc.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@dgieselaar dgieselaar left a comment

Choose a reason for hiding this comment

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

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

@sorenlouv sorenlouv merged commit 79fb749 into elastic:master Oct 10, 2019
@sorenlouv sorenlouv deleted the apm-acm-extended-options branch October 10, 2019 10:16
sorenlouv added a commit that referenced this pull request Oct 10, 2019
… 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
@sorenlouv sorenlouv changed the title [APM] Agent configuration phase 2 [APM] Agent configuration GA Oct 10, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:test-plan-done Pull request that was successfully tested during the test plan release_note:enhancement v7.5.0
Projects
None yet
4 participants