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

[Telemetry] Telemetry Advanced settings section should use the regular UI registration API #56498

Open
Bamieh opened this issue Jan 31, 2020 · 21 comments
Labels
blocked bug Fixes for quality problems that affect the customer experience Feature:Telemetry impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort Project:i18n Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@Bamieh
Copy link
Member

Bamieh commented Jan 31, 2020

We should also look if we can register it in the same way other Advanced Settings are registered (so users can use the Filter by type option as well).


Original content

Describe the feature:

Searching for "telemetry" terms in the advanced settings searchbox are not localized.

When searching advanced settings while using other languages, users must search for the terms in english.

Current searchable terms:

const SEARCH_TERMS = ['telemetry', 'usage', 'data', 'usage data'];

Maybe it is worth implementing this for all advanced settings cc @mattkime @LeeDr

@elasticmachine
Copy link
Contributor

Pinging @elastic/pulse (Team:Pulse)

@afharo
Copy link
Member

afharo commented Dec 10, 2020

We should also look if we can register it in the same way other Advanced Settings are registered (so users can use the Filter by type option as well).

@afharo afharo added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc and removed Team:KibanaTelemetry labels Dec 10, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@afharo afharo added the bug Fixes for quality problems that affect the customer experience label Dec 10, 2020
@afharo
Copy link
Member

afharo commented Dec 10, 2020

Tentatively marking this issue as a bug

@afharo afharo added this to Bug Backlog in kibana-core [DEPRECATED] Dec 17, 2020
@joshdover joshdover changed the title [Telemetry] searching for "telemetry" terms in the advanced settings searchbox are not localized [Telemetry] Telemetry Advance settings section should use the regular UI registration API Jan 14, 2021
@afharo afharo changed the title [Telemetry] Telemetry Advance settings section should use the regular UI registration API [Telemetry] Telemetry Advanced settings section should use the regular UI registration API Jan 21, 2021
@afharo
Copy link
Member

afharo commented May 11, 2021

Q: Is there a way to register UI Settings that are applied globally instead of per-space?

@pgayvallet
Copy link
Contributor

Q: Is there a way to register UI Settings that are applied globally instead of per-space?

No, the config object is namespace-scoped, there is no way to specify a setting as global atm.

export const uiSettingsType: SavedObjectsType = {
name: 'config',
hidden: false,
namespaceType: 'single',

This is an improvement that is probably going to be required for the user profile feature though, as the concept of 'global' versus 'space-aware' settings will probably need to be introduced. cc @TinaHeiligers

@afharo
Copy link
Member

afharo commented May 12, 2021

Cool! Good to know! I'm going to mark this issue as blocked then, while we get there.

@TinaHeiligers
Copy link
Contributor

This is an improvement that is probably going to be required for the user profile feature though, as the concept of 'global' versus 'space-aware' settings will probably need to be introduced. cc @TinaHeiligers

@pgayvallet thanks for the ping! I've added that request to the every-growing list of uiSettings service enhancements.

We need to tackle that list as a team soon (before 8.0.0 gets too close) and organize it in terms of what's still valid & prioritize the enhancements

@flash1293
Copy link
Contributor

before 8.0.0 gets too close

@TinaHeiligers As 8.0 is really close now, is there any update on this? This is causing some weirdness in the advanced settings ui: #56566 #88345

@TinaHeiligers
Copy link
Contributor

@flash1293 this issue isn't on our priority list for 8.0. Since this is already classified as a bug, maybe we can squeeze it in post FF during the testing spike. Would that work for you?

@flash1293
Copy link
Contributor

No pressure from our side, just wanted to raise awareness this is causing some additional downstream problems - I basically stumbled over the 8.0 mention above by accident

@Bamieh
Copy link
Member Author

Bamieh commented Oct 14, 2021

@flash1293 we have custom logic when the switch handler changes. we hit a route in the telemetry plugin to trigger a few things on kibana server. I just want to make sure we are still able to do this if we go ahead with this change

@pgayvallet
Copy link
Contributor

pgayvallet commented Oct 15, 2021

Since this is already classified as a bug, maybe we can squeeze it in post FF during the testing spike. Would that work for you?

Hum, I'm not sure everyone is aware of the implications of the requested change/fix.

  1. The telemetry 'settings' are not what we call advanced settings, and are based on a totally different SO type (telemetry)
  2. The /api/telemetry/v2/optIn endpoint called when the opt-in changes executes custom logic. This is not something supported

These two points are (probably) the very reason why we went with this 'foot component' hack in the first place.

Registering the telemetry section as we do with the real settings (uiSettings) would either:

  • require a whole re-thinking of the advanced settings page, to allow to display 'settings' with other source that the uiSettings, settings with distinct datasource and load/save mechanisms.
  • implement the concept of global uiSettings + allow onChange hooks when registering settings to allow to move the logic currently held in src/plugins/telemetry/server/routes/telemetry_opt_in.ts.

Let's be realistic here, this is not something that will be done in a matter of hours, of even days.

I just want to make sure we are still able to do this if we go ahead with this change

This question is probably on us. Even if still not reflected in CODEOWNERS, core is / will be soon the owner of the advanced_settings plugin.

EDIT:

This is, if we want to answer this issue's title, section should use the regular UI registration API. If we just want to solve the Searching for "telemetry" terms in the advanced settings searchbox are not localized. part of the description, some i18n.translate calls for SEARCH_TERMS should probably do the trick.

But this wouldn't solve the accessibility issue (#56566) @flash1293 linked in #56498 (comment)

@exalate-issue-sync exalate-issue-sync bot added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels Nov 4, 2021
@afharo
Copy link
Member

afharo commented Jul 19, 2022

The telemetry 'settings' are not what we call advanced settings, and are based on a totally different SO type (telemetry)

FWIW, this is not a hard requirement in the telemetry plugin. TBO, it used to be a UI Setting.

I'm more concerned about the global UI Setting and having a hook so we can react to changes so we can call /api/telemetry/v2/optIn.

@pgayvallet
Copy link
Contributor

Yea, global-scoped uiSettings and on-change server-side hooks are both features that would make sense. May be hard to justify implementing them just for 'some tech debt' though 😞 .

If on update hooks should be fairly simple, global-scoped uiSettings are a whole other story, given we need to change all the uiSettings client to aggregate the scoped and unscoped settings (that will have to be 2 distinct SO types and objects by nature)

@afharo
Copy link
Member

afharo commented Sep 1, 2022

We could probably create an internal Shared SO across spaces for those global-scoped uiSettings 😇

@pgayvallet
Copy link
Contributor

We could probably create an internal Shared SO across spaces for those global-scoped uiSettings

Or just use a namespace-agnostic SO type, seems more appropriate tbh!

@majagrubic
Copy link
Contributor

Given global settings support is now available, any chance of moving this to be a "regular" uiSetting?

@pgayvallet
Copy link
Contributor

pgayvallet commented Feb 1, 2023

Given global settings support is now available, any chance of moving this to be a "regular" uiSetting?

Having global UI settings moved us forward, as we have a way to store the telemetry settings. However it's not sufficient, as we still need some kind of hook to react to the change of the setting (see this comment and that one)

@afharo
Copy link
Member

afharo commented Jun 7, 2023

The translation issue was fixed in #158669.

However, the accessibility issue and the fact that we show "No results" but show the section when searching any of the terms is still there.

Last night, I had a quick look at potentially fixing those while keeping the component as a footer, and I don't think we can fix it that way: it works when the Global settings tab is open but not on the Space-aware settings (simply because on the latter, the component is not mounted yet, so the hooks to claim that the search term was found are not evaluated).

I think a UI-hook to react to the change should be enough (no need for a server-side hook). IMO, that's simpler to implement, and we could achieve a similar effect.

I may try this approach during my spare time later today.

@pgayvallet
Copy link
Contributor

I think a UI-hook to react to the change should be enough

Not ideal, but if it's easier, it's probably sufficient for this specific case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked bug Fixes for quality problems that affect the customer experience Feature:Telemetry impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort Project:i18n Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
Development

No branches or pull requests

7 participants