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

Track telemetry implementation for odo #4462

Closed
14 of 17 tasks
valaparthvi opened this issue Feb 25, 2021 · 51 comments · Fixed by #4456, #4580, #4586, #4565 or #4662
Closed
14 of 17 tasks

Track telemetry implementation for odo #4462

valaparthvi opened this issue Feb 25, 2021 · 51 comments · Fixed by #4456, #4580, #4586, #4565 or #4662
Assignees
Labels
estimated-size/M (10-20) Rough sizing for Epics. About 1 sprint of work for one person kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)).

Comments

@valaparthvi
Copy link
Contributor

valaparthvi commented Feb 25, 2021

/kind feature
/priority high

Acceptance Criteria

For first pass -

For later -
Collect information such as -

  • User preferences
  • Proxy enabled

Why is this needed?

#1236

@openshift-ci-robot openshift-ci-robot added kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)). labels Feb 25, 2021
@valaparthvi valaparthvi changed the title Implement telemetry for odo Track telemetry implementation for odo Feb 25, 2021
@valaparthvi valaparthvi linked a pull request Feb 25, 2021 that will close this issue
4 tasks
@kadel
Copy link
Member

kadel commented Feb 25, 2021

This looks like a good start.

Just one note. Don't forget about correctly handling anonymousId (using existing or generating new if needed) as described in Telemetry Guidence document.
You can check how CRC is doing it https://github.com/code-ready/crc/blob/194965b96b3c2f8ed534da1b8bfaeb2e43790612/pkg/crc/segment/segment.go#L117-L135

@deboer-tim
Copy link

Ditto, looks like good coverage. It's implied but the timing/error is really logging every command run with success/failure and time.

I don't recall if odo failure messages sometimes include user info like id or paths. If not then you can just send those (or you can send reason codes like "ran out of space" or "network error") and avoid having to sanitize.

@valaparthvi valaparthvi self-assigned this Mar 1, 2021
@kadel
Copy link
Member

kadel commented Mar 1, 2021

  • User preferences(?)
    • Proxy enabled(?)

I would say that we don't need this at the beginning. Let's keep the initial telemetry simple and small. We can always add more later.

@valaparthvi
Copy link
Contributor Author

Of course. I added them as a placeholder so that I don't forget to discuss them.

@valaparthvi valaparthvi mentioned this issue Mar 4, 2021
4 tasks
@valaparthvi
Copy link
Contributor Author

valaparthvi commented Mar 4, 2021

alias AC=Acceptance Criteria

Scope of this issue for Sprint 198 -

  • Get Add telemetry consent to preference #4456 merged so that AC#1 can be marked as done.
  • Work on getting AC#3.4,4,5 this sprint, if time permits then on the rest of AC#3.
    • AC 3.4
    • AC 3.3
    • AC 4
    • AC 5
  • For AC#5 I have segment code in place(see PR4482) but I am still struggling to send all data to segment because of the placement of function that uploads data to segment segmentClient.Upload is incorrect, it will be fixed once I figure where to hook the function which I think is somewhere in LogErrorAndExit. Currently, it only sends commands that ran successfully or did not break short because of some error.
  • Add test cases.
  • AC 2 - Add prompt
  • AC 7 - Add usage data
  • Once the above is done, I will be able to fix any new errors that pop up.

@dharmit dharmit added the estimated-size/XL (40-60) Rough sizing for Epics. About 3 sprints of work for a person label Mar 4, 2021
@kadel
Copy link
Member

kadel commented Mar 8, 2021

/reopen
only the first item was done

@openshift-ci-robot
Copy link
Collaborator

@kadel: Reopened this issue.

In response to this:

/reopen
only the first item was done

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@valaparthvi
Copy link
Contributor Author

valaparthvi commented Mar 25, 2021

Scope of this issue for Sprint 199 -

  • Merge Enter Telemetry  #4482
  • Add Cluster type information (k8s or OpenShift)
  • Sanitize the data coming into Segment
  • Figure what devfile are users using - java, nodejs, quarkus, maven?
  • Analyze what more data needs to go in and add them.
  • Make segment upload asynchronous.

@dharmit dharmit removed the estimated-size/XL (40-60) Rough sizing for Epics. About 3 sprints of work for a person label Mar 25, 2021
@dharmit
Copy link
Member

dharmit commented Mar 26, 2021

Add Cluster type information (k8s or OpenShift)

I'm interested in this information since I want to understand if how wilde OCP 3.11 is used.

Other than that, I'm also interested in knowing if users use Service Catalog. I'm not sure how/if this information can be fetched. One way could be by looking at the odo service create commands being executed. Other way could be to see if the cluster in question has API objects mentioned here viz. ClusterServiceClass and ClusterServiceBroker.

WDYT @kadel @deboer-tim ?

@valaparthvi
Copy link
Contributor Author

Add Cluster type information (k8s or OpenShift)

@dharmit @girishramnani Can I get a pointer on this one?

@deboer-tim
Copy link

Add Cluster type information (k8s or OpenShift)

I'm interested in this information since I want to understand if how wilde OCP 3.11 is used.

Other than that, I'm also interested in knowing if users use Service Catalog. I'm not sure how/if this information can be fetched. One way could be by looking at the odo service create commands being executed. Other way could be to see if the cluster in question has API objects mentioned here viz. ClusterServiceClass and ClusterServiceBroker.

WDYT @kadel @deboer-tim ?

I've said it before, but once we have basic telemetry for each command we should go ahead and 'ship it'. There are some obvious other things we can add, but this is incremental and we'll think of others later/as we review telemetry. The top things I think I'd want to know are:

  • The kind of project used (i.e. name of parent devfile or s2i).
  • The kind of cluster they're connecting to (i.e. Kind vs Minikube vs CRC vs OpenShift).
  • What kind of service they're creating or binding to.

@valaparthvi
Copy link
Contributor Author

Consenting to Telemetry makes odo significantly slower. Currently, 2 messages are sent to Segment, 1) Identify - that contains system information and, 2) Track - that contains information about the executed command.

To mend this @kadel and I discussed a few solutions, which I am listing here -

  1. Tomas suggested that the reason this is slow is that it takes time to open the client and close it, the amount of data sent does not affect the speed much. So we can send some data before the command runs, i.e. once the command is validated, we send the Track message and then let odo do its thing. Since sending the message is asynchronous, it will not affect odo's execution, and there won't be any delay that we see at the end. This also means that we will not send information about the success of a command. But in case the command fails, we will send a message about it, in which case, there will be some delay at the end.

  2. Run a daemon process that sends the data to segment even after the actual odo execution has finished.

I think the first solution seems to the optimum one. I am going to test it and see if it helps.

There are 2 things that we will need to change regardless of the solution we go with -

  1. Set BatchSize to 1 while instantiating a new segment Client so that it sends a message as soon it is queued.
  2. It does not make sense to send Identify message with every command since this information is included in the Track message. So we will only send the Identify message when a new anonymousID is created.

Tomas, I feel like I am missing one other thing that we discussed, but I hope not.

@valaparthvi
Copy link
Contributor Author

@kadel The first implementation mentioned here #4462 (comment) when implemented with BatchSize=1 and Interval=1Millisecond seems to be working fine with long running commands such odo create or odo push, but it still has the same delay with fast commands such as odo preference view or odo config view. IMO it's perhaps still better than what it is right now.
Perhaps we can skip uploading the data for a command that does view or list operations or do you think that data is important?

@valaparthvi
Copy link
Contributor Author

Reopening. #4662 only fixed a part of this issue.

@valaparthvi valaparthvi reopened this May 20, 2021
@valaparthvi
Copy link
Contributor Author

valaparthvi commented May 27, 2021

Scope of the issue for sprint 202 -

@valaparthvi
Copy link
Contributor Author

valaparthvi commented Jun 16, 2021

Scope for 203 -

@valaparthvi
Copy link
Contributor Author

Cluster ID can be added on OCP4 by fetching the clusterversion - oc get clusterversion -o jsonpath='{.items[].spec.clusterID}{"\n"}', but this is not possible if it is a non-admin user.

Same thing is for K8s. I am yet to check for OCP3, but I suspect the same issue with it.

@dharmit dharmit added estimated-size/M (10-20) Rough sizing for Epics. About 1 sprint of work for one person and removed points/3 labels Jun 17, 2021
@valaparthvi
Copy link
Contributor Author

@dharmit Is it alright if I close this issue now? We have everything for telemetry in place and anything that pops up now can be tracked in a separate issue.

@dharmit
Copy link
Member

dharmit commented Jul 6, 2021

/close

@openshift-ci openshift-ci bot closed this as completed Jul 6, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jul 6, 2021

@dharmit: Closing this issue.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
estimated-size/M (10-20) Rough sizing for Epics. About 1 sprint of work for one person kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)).
Projects
None yet
5 participants