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.

Sign up for GitHub

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

Jump to bottom

[Telemetry] Use staging if it's not a distributable release #63875

Merged
merged 3 commits into from
Apr 20, 2020
Merged

[Telemetry] Use staging if it's not a distributable release #63875

merged 3 commits into from
Apr 20, 2020

Conversation

afharo
Copy link
Member

@afharo afharo commented Apr 17, 2020

Summary

At the moment we rely on the dev flag to decide whether to push the telemetry to staging or production.
We can use the dist flag instead. This flag is true only when running from a distributable (either built from source or downloaded), but not when running as node scripts/kibana start.

Maybe we can get confirmation this is the expected behaviour from the @elastic/kibana-operations team.

For maintainers

@afharo afharo added Team:Operations Team label for Operations Team Feature:Telemetry v8.0.0 release_note:skip Skip the PR/issue when compiling release notes :Telemetry v7.8.0 labels Apr 17, 2020
@afharo afharo requested a review from a team as a code owner April 17, 2020 16:45
@elasticmachine
Copy link
Contributor

Pinging @elastic/pulse (Team:Pulse)

@afharo
Copy link
Member Author

afharo commented Apr 17, 2020

During the build process, another flag is set: release. It is false for snapshots and true only for final releases. I don't think we want to use that flag at the moment (we may want customers running nightly builds to push data to our production cluster), or do we?
@alexfrancoeur what are your thoughts on this?

spalger
spalger reviewed Apr 17, 2020
View reviewed changes
src/plugins/telemetry_collection_manager/server/plugin.ts Outdated
private readonly version: string;

constructor(initializerContext: PluginInitializerContext) {
this.logger = initializerContext.logger.get();
this.isDev = initializerContext.env.mode.dev;
this.isProd = initializerContext.env.packageInfo.dist; // Is it a distributable release? => isProd
Copy link
Contributor

@spalger spalger Apr 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

"prod" and "dev" mean something different than "dist" and "source", this probably shouldn't be called isProd

@Bamieh
Copy link
Member

Bamieh commented Apr 20, 2020

What is the difference between prod and dist?

@afharo
Copy link
Member Author

afharo commented Apr 20, 2020

What is the difference between prod and dist?

@Bamieh from here:
https://github.com/elastic/kibana/blob/master/src/core/server/config/env.ts#L122-L136

prod and dev are uniquely based on whether Kibana is run in --dev mode (either via env var or via cli flag).
dist is based on the flag package.json>build.distributable, only set when running node scripts/build. Either if we release it or it's compiled from source, dist: true, but when running Kibana as node scripts/kbn or yarn kbn, dist: false.

Anyway, I'll update in a sec this PR about @spalger 's comment. He is right: we shouldn't mix their names :)

@afharo
Copy link
Member Author

afharo commented Apr 20, 2020

I'm wondering if these changes mean we can revert the ones from this PR: #63356

On one hand, I'd rather keep it so we don't populate staging with (potentially broken) garbage, as PRs might contain malformed documents.
On the flip side, I think we should report telemetry during the builds (to staging, of course) because that could bring up potential crashes.

Actually, the potentially broken garbage reason has never been a problem when pushing the data to Live and during the dev time, those documents are likely to be pushed to staging anyway (when the dev is running Kibana locally).

Before pushing a commit reverting those changes, I'd like to know your thoughts @elastic/pulse ?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Bamieh
Bamieh approved these changes Apr 20, 2020
View reviewed changes
Copy link
Member

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

LGTM. I still think we should keep the enforced staging urls for testing in any case.

@afharo afharo merged commit 9226d4c into elastic:master Apr 20, 2020
@afharo afharo deleted the telemetry/isProd-based-on-distributable branch April 20, 2020 11:43
@afharo afharo mentioned this pull request Apr 20, 2020
Merged
afharo added a commit to afharo/kibana that referenced this pull request Apr 20, 2020
@afharo
[Telemetry] Use staging if it's not a distributable release (elastic#…
4af7b73 
…63875)

* [Telemetry] Use staging if it's not a distributable release (instead of 'dev' vs. 'prod' approach)

* Rename isProd to isDistributable and useProdKey
afharo added a commit that referenced this pull request Apr 20, 2020
@afharo
[Telemetry] Use staging if it's not a distributable release (#63875) (#…
4bde4a8 
…63955)

* [Telemetry] Use staging if it's not a distributable release (instead of 'dev' vs. 'prod' approach)

* Rename isProd to isDistributable and useProdKey
@lukeelmers lukeelmers added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Oct 1, 2021
@elasticmachine
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spalger spalger spalger left review comments

@Bamieh Bamieh Bamieh approved these changes

Assignees
No one assigned
Labels
Feature:Telemetry release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team v7.8.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@afharo @elasticmachine @Bamieh @kibanamachine @spalger @lukeelmers