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

Metrics settings uses the Logs UI capabilities #48180

Closed
kobelb opened this issue Oct 14, 2019 · 8 comments · Fixed by #49781
Closed

Metrics settings uses the Logs UI capabilities #48180

kobelb opened this issue Oct 14, 2019 · 8 comments · Fixed by #49781
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Feature:Metrics UI Metrics UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services

Comments

@kobelb
Copy link
Contributor

kobelb commented Oct 14, 2019

Currently, Metrics and Logs share the infrastructure-ui-source saved-object for storing their settings. However, prior to 7.4 when the end-user had read access to Metrics and write-access to Logs, they were able to update the source configuration only in the Logs application not in the Metrics application. This is no longer the case in 7.4, and both the Logs and Metrics use of the settings page use the logs ui capabilities.

@kobelb kobelb added bug Fixes for quality problems that affect the customer experience Feature:Metrics UI Metrics UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services labels Oct 14, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-logs-ui (Team:infra-logs-ui)

@Zacqary
Copy link
Contributor

Zacqary commented Oct 21, 2019

What is acceptance criteria for this issue? Do we need to give the Metrics application a different settings page?

Screen Shot 2019-10-21 at 11 32 05 AM

The Metrics index pattern is still configurable from this page; do we want to omit all logs-related settings?

@weltenwort
Copy link
Member

No, this refers to the fact that there is a separate capability infrastructure.configureSource, which used to be checked to control whether the source configuration form was read-only or writable back when it was still in a fly-out:

shouldAllowEdit={uiCapabilities.infrastructure.configureSource as boolean}

Somewhere along the way we accidentally lost that so that on both settings tabs we check the logs capabilities. That those share an underlying saved object is a separate issue that is tracked in #31287.

This is about reconstituting the UI checks to a comparable effect before the settings tab refactoring.

@Zacqary Zacqary self-assigned this Oct 30, 2019
@Zacqary
Copy link
Contributor

Zacqary commented Oct 30, 2019

So we want to use infrastructure.configureSource for when you click the Settings tab from the Metrics UI, and logs.configureSource from the Logs UI, correct?

I'm still worried about what happens if the logs. and infrastructure. values are different. Let's say logs.configureSource is false, but this change would mean that Logs settings are only read-only if you access them through the Logs settings tab; accessing them from the Metrics settings tab would allow you to change them.

Should we instead be refactoring this into two shouldAllowEdit flags? One for logs, one for metrics? And disable the relevant editable fields if one of those flags is false?

@weltenwort
Copy link
Member

To restore the original functionality a single flag would suffice. But I like the idea of disabling some fields in the settings form. In the interest of code cleanliness I would vote for implementing it as a single prop that can take three values instead of two booleans.

interface SourceConfigurationSettingsProps {
  editableSettings: 'metricsSettings' | 'logsSettings' | 'none';
}

That means we could split the SettingsPage into a MetricsSettingsPage and a LogsSettingsPage (which could be put into pages/metrics and pages/logs respectively). Each of them could fetch the corresponding capability and pass a tri-state prop to the SourceConfigurationSettings component:

/// in MetricsSettingsPage
<SourceConfigurationSettings editableSettings={uiCapabilities.metrics.configureSource ? 'metricsSettings' : 'none'} />
/// in LogsSettingsPage
<SourceConfigurationSettings editableSettings={uiCapabilities.logs.configureSource ? 'logsSettings' : 'none'} />

@Zacqary what do you think?

@Zacqary
Copy link
Contributor

Zacqary commented Oct 30, 2019

I'd have to refactor a bunch of components, like the readOnly flag that controls both Metrics and Logs indices.

Do we want both Metrics and Logs fields to still be visible on both pages? Or do we want to make only Metrics-related ones visible on Metrics, and vice versa?

@Zacqary
Copy link
Contributor

Zacqary commented Oct 30, 2019

Screen Shot 2019-10-30 at 4 35 38 PM

Are any of these relevant to Metrics? Or are they all just Logs params?

@weltenwort
Copy link
Member

This is the relevance as I remember it:

Field Logs Metrics
Timestamp ✔️ ✔️
Tiebreaker ✔️
Container ID ✔️
Host name ✔️
Pod ID ✔️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Metrics UI Metrics UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants