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.

Sign up for GitHub

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

Jump to bottom

[APM] Remote Agent Config: Add additional (java) options #59860

Merged
merged 37 commits into from
Mar 23, 2020
Merged

[APM] Remote Agent Config: Add additional (java) options #59860

merged 37 commits into from
Mar 23, 2020

Conversation

sorenlouv
Copy link
Member

@sorenlouv sorenlouv commented Mar 11, 2020 •
edited
Loading

Closes: elastic/apm#214

TODO:

  • Agent specific options (only show option for specific services)
  • When selecting "All" services, only show options that are relevant to all backend agents (not considering RUM)
  • Highlight "Agent configuration" menu item when creating/editing config
  • Options are sorted alphabetically
  • Limit options by to specific agents (eg java specific options)
  • Discard button on settings page should reset settings
  • Cancel button on service page should take user to the list page (overview)
  • Bottom bar overflows the left hand navigation (waiting for EUI to add a prop, or allow arbitrary css) (Interoperability of EuiBottomBar and EuiNavDrawer eui#3146)
  • Review page (not for 7.7)
  • Config per agent name (not for 7.7)
  • Categories: list options under categories (not for 7.7)
  • Highlight previously edited options (not for 7.7)

Questions:

  • Are the descriptions the same across all agents?
  • Are the default values the same across all agents?

image

image

image

image

@sorenlouv sorenlouv marked this pull request as ready for review March 11, 2020 15:05
@sorenlouv sorenlouv requested a review from a team as a code owner March 11, 2020 15:05
sorenlouv
sorenlouv commented Mar 11, 2020
View reviewed changes
x-pack/legacy/plugins/apm/public/components/app/Main/route_config/index.tsx Outdated
),
name: RouteName.AGENT_CONFIGURATION_CREATE,
component: () => {
const { search } = history.location;
Copy link
Member Author

@sorenlouv sorenlouv Mar 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Controversy #1: retrieving search from history singleton. Everywhere else we get it from useLocation hook.
The problem with hooks is that they only work inside react components (not vanilla js) so in general I think when given the choice it's better to use the singleton approach (for consistency).

afact there are no difference between the two approaches. LMK if there are!

sorenlouv
sorenlouv commented Mar 11, 2020
View reviewed changes
...pp/Settings/AgentConfigurations/AgentConfigurationCreateEdit/SettingsPage/SettingFormRow.tsx
return (
<EuiFieldNumber
placeholder={setting.placeholder || defaultInputPlaceholder}
value={(value as any) || ''}
Copy link
Member Author

Choose a reason for hiding this comment

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

EuiFieldNumber currently don't accept strings. This will change with this PR

@sorenlouv sorenlouv force-pushed the agent-configuration-additional-settings branch from 41032e6 to 259f363 Compare March 11, 2020 23:32
@sorenlouv sorenlouv force-pushed the agent-configuration-additional-settings branch 2 times, most recently from b8cab16 to 44f18c8 Compare March 16, 2020 09:20
sorenlouv and others added 12 commits March 22, 2020 09:02
@sorenlouv
Add type
8fdbeb6
@sorenlouv
Undo changes tp url_helpers
39fa662
@sorenlouv
Only show options that are applicable to all backend agents
3be4d53
@sorenlouv
Highlight menu item
096b4a2
@sorenlouv
Add floating point number and java settings
d209def
@sorenlouv
Split settings into “general” and “”java
a646575
@sorenlouv
rename “configSettingDefinitions” to “settingDefinitions”
e4fe71e
@sorenlouv
Fix tests
c30ace4
@sorenlouv
fix duplicated i18n labels
2702fbf
@sorenlouv @formgeist
Update x-pack/legacy/plugins/apm/public/components/app/Settings/Agent…
58a1dee 
…Configurations/AgentConfigurationCreateEdit/ServicePage/ServicePage.tsx

Co-Authored-By: Casper Hübertz <casper@formgeist.com>
@sorenlouv @formgeist
Update x-pack/legacy/plugins/apm/public/components/app/Settings/Agent…
9dea4c7 
…Configurations/AgentConfigurationCreateEdit/ServicePage/ServicePage.tsx

Co-Authored-By: Casper Hübertz <casper@formgeist.com>
@sorenlouv @formgeist
Update x-pack/legacy/plugins/apm/public/components/app/Settings/Agent…
cb51fcc 
…Configurations/AgentConfigurationCreateEdit/ServicePage/ServicePage.tsx

Co-Authored-By: Casper Hübertz <casper@formgeist.com>
@sorenlouv sorenlouv force-pushed the agent-configuration-additional-settings branch from cc73feb to cb51fcc Compare March 22, 2020 08:05
@sorenlouv
Copy link
Member Author

If you have unsaved changes (which is the current behaviour), wouldn't it make more sense to cancel the edits and return to the create page where you started instead of sending the user back to the list page?

@formgeist

I'll change the Cancel button to be a Discard button while in edit mode. It will behave as described above.

WDYT should happen when creating a new config?

@formgeist
Copy link
Contributor

WDYT should happen when creating a new config?

If we're keeping the "discard" changes button even on a new config, the user would have to go back to the overview page via the sidenav to "cancel", as there's no specified cancel option. I don't think that's too big of an issue.

cauemarcondes
cauemarcondes approved these changes Mar 23, 2020
View reviewed changes
Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@sorenlouv sorenlouv merged commit 6535985 into elastic:master Mar 23, 2020
@sorenlouv sorenlouv mentioned this pull request Mar 23, 2020
Merged
@sorenlouv sorenlouv deleted the agent-configuration-additional-settings branch March 23, 2020 21:33
sorenlouv added a commit that referenced this pull request Mar 23, 2020
@sorenlouv
[APM] Remote Agent Config: Add additional (java) options (#59860) (#6…
fe73e93 
…0973)
@jalvz jalvz mentioned this pull request Mar 24, 2020
Closed
@sorenlouv sorenlouv mentioned this pull request May 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmorelli25 bmorelli25 bmorelli25 left review comments

@formgeist formgeist formgeist approved these changes

@cchaos cchaos cchaos left review comments

@graphaelli graphaelli graphaelli left review comments

@smith smith smith requested changes

@cauemarcondes cauemarcondes cauemarcondes approved these changes

Assignees
No one assigned
Labels
apm:Agent Configuration release_note:enhancement v7.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[APM] Remote agent configuration: Update flyouts to feature new config options
8 participants
@sorenlouv @formgeist @kibanamachine @smith @graphaelli @cchaos @bmorelli25 @cauemarcondes