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

[Uptime] Add Settings Page #53550

Merged
merged 103 commits into from
Mar 21, 2020
Merged

[Uptime] Add Settings Page #53550

merged 103 commits into from
Mar 21, 2020

Conversation

andrewvc
Copy link
Contributor

@andrewvc andrewvc commented Dec 19, 2019

Summary

Adds a settings page to the Uptime UI. The settings page values are per-space. The only current setting is heartbeatIndices.

To test this against alternate indices try changing setup.ilm.rollover_alias in heartbeat.yml to something like alt-prefix. See the ilm docs for more details.

This should be tested with read-only and write only roles. To test this in kibana try creating two users with two different roles in kibana. One roll should have read access to the Uptime space in kibana. The other should have all access. Both should have read permissions for the heartbeat-* index pattern.

This patch also splits API perms from just heartbeat to uptime-read and uptime-write.

This patch also refactors some of the header component functionality, using hooks for breadcrumbs, and making the top links optional.

Fixes elastic/uptime#43

image

image

image

image

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@andrewvc andrewvc added WIP Work in progress enhancement New value added to drive a business result Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Dec 19, 2019
@andrewvc andrewvc self-assigned this Dec 19, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@andrewvc andrewvc added release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Dec 19, 2019
*/
export const getSourceSettings = async (basePath?: string, setter?: (data: unknown) => void) => {
try {
const { data } = await axios.get(getApiPath('/api/uptime/source_settings', basePath));
Copy link
Contributor

@mshustov mshustov Dec 19, 2019

Choose a reason for hiding this comment

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

Is there a reason not to use http.fetch instead? Our Security model heavily depends on the fact that every plugin uses core provided API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the pointer, will make the switch, this file was copied from some much older code pre NP

Copy link
Contributor Author

@andrewvc andrewvc Mar 5, 2020

Choose a reason for hiding this comment

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

I think we need to do this across the whole app. We're tracking that here. I'll leave this PR as-is since we have to make a change everywhere ultimately. #56894

* you may not use this file except in compliance with the Elastic License.
*/

export { getSourceSettings } from './get_source_settings';
Copy link
Contributor

@mshustov mshustov Dec 19, 2019

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can do

@elasticmachine
Copy link
Contributor

💔 Build Failed

History

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

@andrewvc
Copy link
Contributor Author

@elasticmachine merge upstream

): Promise<MonitorSummaryResult> {
const dynamicSettings = await savedObjectsAdapter.getUptimeDynamicSettings(
savedObjectsClient,
undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

why you have to pass undefined?

Copy link
Contributor

Choose a reason for hiding this comment

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

i see the same case in other places as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! The type required a second argument for no reason.

Comment on lines +58 to +59
saveSucceeded: false,
saveError: action.payload,
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like typing isn't working correctly here, otherwise it should have thrown error on saveSucceeded, something we can improve for redux reducer. but unrelated to this PR.

Comment on lines +35 to +58
const couldNotSaveSettingsText = i18n.translate('xpack.uptime.settings.error.couldNotSave', {
defaultMessage: 'Could not save settings!',
});
yield takeLatest(String(setDynamicSettings), function*(action: Action<DynamicSettings>) {
try {
if (!action.payload) {
const err = new Error('Cannot fetch effect without a payload');
yield put(setDynamicSettingsFail(err));

kibanaService.core.notifications.toasts.addError(err, {
title: couldNotSaveSettingsText,
});
return;
}
const basePath = yield select(getBasePath);
yield call(setDynamicSettingsAPI, { settings: action.payload, basePath });
yield put(setDynamicSettingsSuccess(action.payload));
kibanaService.core.notifications.toasts.addSuccess('Settings saved!');
} catch (err) {
kibanaService.core.notifications.toasts.addError(err, {
title: couldNotSaveSettingsText,
});
yield put(setDynamicSettingsFail(err));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nice usage of toasts here.

Comment on lines 35 to 36
return await apiService.post(apiPath, JSON.stringify(settings), DynamicSettingsSaveType);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder, if you have to do JSON.stringify? doesn't kibana http post takes care of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope! Removed, good fix :)

Comment on lines 36 to 40
useEffect(() => {
if (setBreadcrumbs) {
setBreadcrumbs([makeBaseBreadcrumb(params)].concat(extraCrumbs));
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

you can optimize it by passing effect depencies array, that way it will be only called when extracrumbs really changes, right now this will be called each time component renders.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

Code Looks good, just left some minor optional comments. WFG !!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@andrewvc andrewvc merged commit c2e57af into elastic:master Mar 21, 2020
@andrewvc andrewvc deleted the source-settings branch March 21, 2020 23:13
andrewvc added a commit to andrewvc/kibana that referenced this pull request Mar 22, 2020
Adds a settings page to the Uptime UI. The settings page values are per-space. The only current setting is heartbeatIndices.

To test this against alternate indices try changing setup.ilm.rollover_alias in heartbeat.yml to something like alt-prefix. See the ilm docs for more details.

This should be tested with read-only and write only roles. To test this in kibana try creating two users with two different roles in kibana. One roll should have read access to the Uptime space in kibana. The other should have all access. Both should have read permissions for the heartbeat-* index pattern.

This patch also splits API perms from just heartbeat to uptime-read and uptime-write.

This patch also refactors some of the header component functionality, using hooks for breadcrumbs, and making the top links optional.

Fixes elastic/uptime#43
andrewvc added a commit that referenced this pull request Mar 22, 2020
* [Uptime] Add Settings Page (#53550)

Adds a settings page to the Uptime UI. The settings page values are per-space. The only current setting is heartbeatIndices.

To test this against alternate indices try changing setup.ilm.rollover_alias in heartbeat.yml to something like alt-prefix. See the ilm docs for more details.

This should be tested with read-only and write only roles. To test this in kibana try creating two users with two different roles in kibana. One roll should have read access to the Uptime space in kibana. The other should have all access. Both should have read permissions for the heartbeat-* index pattern.

This patch also splits API perms from just heartbeat to uptime-read and uptime-write.

This patch also refactors some of the header component functionality, using hooks for breadcrumbs, and making the top links optional.

Fixes elastic/uptime#43

* Fix tests to use heartbeat-7 instead of heartbeat-8
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 23, 2020
* master: (39 commits)
  [APM]Create custom link from Trace summary (elastic#59648)
  [ML] Fixing app clean up (elastic#60853)
  [SIEM] Use ECS categorisation for Authentication widgets (elastic#60734)
  [NP] Remove kbnUrl usage in discover/dashboard/visualize (elastic#60016)
  Skip failing test
  [Uptime]Update fetch effect failed action handling (elastic#60742)
  [npm] upgrade elastic/maki (elastic#60829)
  [Uptime] Add Settings Page (elastic#53550)
  [APM] service maps: avoid unnecesary `useDeepObjectIdentity` (elastic#60836)
  [Index management] Re-enable index template tests (elastic#60780)
  Fixed UI/UX issues: alerts delete confirmation, combobox behaviors (elastic#60703)
  [SIEM] Fix patching of ML Rules (elastic#60830)
  [APM] Service Map - Separate overlapping edges by rotating nodes (elastic#60477)
  [Alerting] fix flaky test for index threshold grouping (elastic#60792)
  [SIEM][Detection Engine] Adds test scripts for machine learning feature
  Flatten child api response for resolver (elastic#60810)
  Change "url" to "urls" in APM agent instructions (elastic#60790)
  [DOCS] Updates API requests and examples (elastic#60695)
  [SIEM] [Cases] Create case from timeline (elastic#60711)
  [Lens] Resetting a layer generates new suggestions (elastic#60674)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result release_note:enhancement Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Uptime] Uptime support for dynamic settings / index names
8 participants