-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] expose getIsOptedIn function in plugin start contract #75143
[telemetry] expose getIsOptedIn function in plugin start contract #75143
Conversation
Pinging @elastic/kibana-telemetry (Team:KibanaTelemetry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we want to publicly expose the null
behaviour. It seems very explicit to our own usage, doesn't it?.
Also, NIT: can we add some tests for this new piece of logic?
…ose_start_contract_opt_in_status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
NIT: It'd be great if we had some coverage for this API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need unit tests for the new logic and also need to expose the plugin mock. Security will be using this new API and will most likely add tests for the implementation thereof.
@Bamieh There's an additional request to expose the telemetry url that came in through an email:
If we can add it to this PR, let's do that. Otherwise we'll explicitly as for a new issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good thus far.
…ose_start_contract_opt_in_status
…ose_start_contract_opt_in_status
Addressed requested changes. exposed url and added plugin mocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks great to me and I love it that we have a mock as well 🚀
I'm missing some unit tests to cover these new methods though (I'm happy to help on implementing them if you want 🙂).
I also added a comment in regards to the docs: Should we make it clear in some way that we'll only return false
in the change of major/minor versions if the previous response was to opt-out?
const telemetrySavedObject = await getTelemetrySavedObject(internalRepository!); | ||
const config = await this.config$.pipe(take(1)).toPromise(); | ||
const allowChangingOptInStatus = config.allowChangingOptInStatus; | ||
const configTelemetryOptIn = typeof config.optIn === 'undefined' ? null : config.optIn; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: I recently learned this can be also written as:
const configTelemetryOptIn = typeof config.optIn === 'undefined' ? null : config.optIn; | |
const configTelemetryOptIn = config.optIn ?? null; |
Co-authored-by: Alejandro Fernández Haro <afharo@gmail.com>
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
…' of github.com:jloleysens/kibana into ilm/fix/pre-existing-policy-with-no-existing-repository * 'ilm/fix/pre-existing-policy-with-no-existing-repository' of github.com:jloleysens/kibana: fix empty string in selected indices (elastic#76855) [Security Solution] Refactor OverviewHost and OverviewNetwork to use Search Strategy (elastic#76409) Use Search API in Timelion (sync) (elastic#75115) [telemetry] expose getIsOptedIn function in plugin start contract (elastic#75143) [ILM] Clean up remaining js files and any typings (elastic#76803) [Logs UI] Shared `<LogStream />` component (elastic#76262) [Security Solution] Add unit test for all hosts (elastic#76752) [Security Solution] Add unit test for authentications search strategy (elastic#76665) Do not apply search source data for tsvb (elastic#75137) [Security Solution] Refactor NetworkDns to use Search Strategy (elastic#76250) [SECURITY SOLUTION] Adds 'cypress:open-as-ci' command (elastic#76125) [Logs UI] Update alert executor tests (elastic#75764) [Functional] Unskip vega tests and fix flakiness (elastic#76600) [Data] Query String Input accepts classname prop (elastic#76848) [ML] Swim lane pagination for viewing by job id (elastic#76847) [Security Solution] Refactor MatrixHistogram to use Search Strategy (elastic#76603) [APM] Use the outcome field to calculate the transaction error rate chart (elastic#75528) [APM] Use observer.hostname instead of observer.name (elastic#76074) Legacy logging: fix remoteAddress being duplicated in userAgent field (elastic#76751)
…s-for-710 * 'master' of github.com:elastic/kibana: [Search] Use async es client endpoints (elastic#76872) fix empty string in selected indices (elastic#76855) [Security Solution] Refactor OverviewHost and OverviewNetwork to use Search Strategy (elastic#76409) Use Search API in Timelion (sync) (elastic#75115) [telemetry] expose getIsOptedIn function in plugin start contract (elastic#75143) [ILM] Clean up remaining js files and any typings (elastic#76803) [Logs UI] Shared `<LogStream />` component (elastic#76262) [Security Solution] Add unit test for all hosts (elastic#76752) [Security Solution] Add unit test for authentications search strategy (elastic#76665) Do not apply search source data for tsvb (elastic#75137) [Security Solution] Refactor NetworkDns to use Search Strategy (elastic#76250) [SECURITY SOLUTION] Adds 'cypress:open-as-ci' command (elastic#76125) [Logs UI] Update alert executor tests (elastic#75764) [Functional] Unskip vega tests and fix flakiness (elastic#76600) [Data] Query String Input accepts classname prop (elastic#76848) [ML] Swim lane pagination for viewing by job id (elastic#76847) [Security Solution] Refactor MatrixHistogram to use Search Strategy (elastic#76603) [APM] Use the outcome field to calculate the transaction error rate chart (elastic#75528) [APM] Use observer.hostname instead of observer.name (elastic#76074) Legacy logging: fix remoteAddress being duplicated in userAgent field (elastic#76751) # Conflicts: # x-pack/plugins/index_lifecycle_management/__jest__/components/edit_policy.test.tsx # x-pack/plugins/index_lifecycle_management/common/types/index.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/node_allocation.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/phases/warm_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/services/api.ts
Pinging @elastic/kibana-core (Team:Core) |
Add
getIsOptedIn
to the start contract. We cannot expose this function to the setup function since SOsavedObjects.createInternalRepository()
is not exposed in setup.Add
getTelemetryUrl
to the setup contract.Add server plugin jest mocks
Closes #68942