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

feat: Add telemetry to track chectl command usage #992

Closed
wants to merge 108 commits into from
Closed

feat: Add telemetry to track chectl command usage #992

wants to merge 108 commits into from

Conversation

flacatus
Copy link
Collaborator

@flacatus flacatus commented Nov 17, 2020

Signed-off-by: Flavius Lacatusu flacatus@redhat.com

What does this PR do?

This PR introduce to chectl the ability to collect and send to segment commands usage and flags:

  • I create hooks which are executed in every command and emit the analytic hook
  • Created a segment adapter to send the usage data to segment
  • Cli Prompt to ask user if allows chectl to collect anonymous data and send to segment
  • Store the user confirmation of usage data into chectl config directory. Is usefull to not ask user in every command if allow chectl to collect data

Refference issue:eclipse-che/che#18069

Screenshot/screencast of this PR

What issues does this PR fix or reference?

How to test this PR?

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
@flacatus
Copy link
Collaborator Author

flacatus commented Nov 17, 2020

Some examxple of how the events are tracked in segment:
image
image

@flacatus
Copy link
Collaborator Author

Pending to fix the tests with the new changes

@tolusha
Copy link
Collaborator

tolusha commented Nov 17, 2020

I have two general remarks.

  1. I am worried about tracking flags. They might contains some personal user's data like accessToken.
    Does it make sense to track only those flags that we are interested in ? like platform ?

  2. We need a general configfile like configuration.json and store segment configuration there.
    @AndrienkoAleksandr @mmorhun WDYT?

@tolusha
Copy link
Collaborator

tolusha commented Nov 17, 2020

  1. What if description is changed, would we have a new event ?

@flacatus
Copy link
Collaborator Author

  1. What if description is changed, would we have a new event ?

Yes, if the description change we will have a new event. Also we can put in the event the command name not the command description

@flacatus
Copy link
Collaborator Author

flacatus commented Nov 18, 2020

@davidfestal @l0rd We are thinking to add an additional flag for every command in part to use telemetry. The flag can be something like: --telemetry=on/off by default to off... In case if the flag will not be turned on chectl cli will ask user from command line if allow chectl to collect anonymous data and save the prompt confirmation into the chectl cache to not ask for every command. It is necessary from my point of view to add this flag because in our general QE jobs will be hard to interact with chectl... WDYT?
cc @tolusha @rhopp

@mmorhun
Copy link
Contributor

mmorhun commented Nov 18, 2020

I am worried about tracking flags. They might contains some personal user's data like accessToken.
Does it make sense to track only those flags that we are interested in ? like platform ?

I think it would be better to have a list of flag that we shouldn't track (like tokens, password, etc.), however I don't know about implementation details here.

We need a general configfile like configuration.json and store segment configuration there.

I am definitely +1 here.

What if description is changed, would we have a new event

I think the ID should be the command (e.g. server:deploy) or command + flag (e.g. server:deploy.platform)

Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
src/commands/auth/get.ts Outdated Show resolved Hide resolved
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
@flacatus flacatus requested a review from tolusha January 4, 2021 08:59
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
src/hooks/analytics/analytics.ts Outdated Show resolved Hide resolved
src/hooks/analytics/analytics.ts Show resolved Hide resolved
src/hooks/analytics/analytics.ts Show resolved Hide resolved
src/hooks/analytics/analytics.ts Show resolved Hide resolved
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
@flacatus
Copy link
Collaborator Author

flacatus commented Jan 4, 2021

/retest

Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
@openshift-ci-robot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: flacatus, tolusha
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@flacatus
Copy link
Collaborator Author

flacatus commented Jan 5, 2021

closing in favor of #1052 after for was broken...

@flacatus flacatus closed this Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants