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

[discuss][EBT] Defer sendTo setup #129014

Closed
Tracked by #121992
afharo opened this issue Mar 31, 2022 · 3 comments · Fixed by #130696
Closed
Tracked by #121992

[discuss][EBT] Defer sendTo setup #129014

afharo opened this issue Mar 31, 2022 · 3 comments · Fixed by #130696
Labels
discuss Feature:Telemetry impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:medium Medium Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@afharo
Copy link
Member

afharo commented Mar 31, 2022

In #128407 (comment) we identified an important limitation in the Kibana architecture that may affect the EBT client:

The client requires the option sendTo: 'production' | 'staging'. However, this information is available based on the telemetry.sendUsageTo config. The client is instantiated in core, meaning that at the point of registration we can't rely on that setting.

Here are the options we can consider:

Read telemetry.sendUsageTo from core

core can actually read telemetry.sendUsageTo from the config. It's the most reliable way because the value is set from the beginning and we don't risk having an initial value different to the final one, meaning some events may be shipped to a different environment depending on when they were generated.

However, it creates an implicit dependency from core to the telemetry plugin, breaking Kibana's architectural principles and the separation of domains we stand for.

Add sendTo config to the registerShipper API

On top of providing it to the client's constructor, we can overwrite this value when registering the shipper. The problem is similar to the previous option: while core doesn't depend on telemetry, other plugins registering their own shippers may depend on telemetry to read this value.

It also adds another problem: different shippers may send to different environments.

Add a new sendTo API

Adding a new dedicated sendTo API to set this parameter would allow us to set the environment once we resolve the config. However, it creates more complexity for the client with another hold-until-resolved mechanism similar to the optIn API.

Also adding the similar risks: if never set, it holds the logic forever. Although these risks could be solved by not allowing to disable the telemetry plugin (deprecate telemetry.enabled: false).

@afharo afharo added discuss Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Telemetry labels Mar 31, 2022
@elasticmachine
Copy link
Contributor

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

@afharo afharo added loe:medium Medium Level of Effort impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. labels Mar 31, 2022
@Bamieh
Copy link
Member

Bamieh commented Mar 31, 2022

Also adding the similar risks: if never set, it holds the logic forever. Although these risks could be solved by not allowing to disable the telemetry plugin (deprecate telemetry.enabled: false).

we can default to staging and just expose a setter/getter for sendTo? Although if telemetry is disabled we're not going to send EBT anyways right?

@afharo
Copy link
Member Author

afharo commented Mar 31, 2022

Although if telemetry is disabled we're not going to send EBT anyways right?

That is correct. Since it won't call the optIn API, the EBT client will keep its unknown consent status, meaning it won't send anything.

@afharo afharo linked a pull request Apr 30, 2022 that will close this issue
2 tasks
@exalate-issue-sync exalate-issue-sync bot reopened this May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Feature:Telemetry impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:medium Medium Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants