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

make perfomrance pr-bot to use json output #9939

Closed
medyagh opened this issue Dec 12, 2020 · 12 comments
Closed

make perfomrance pr-bot to use json output #9939

medyagh opened this issue Dec 12, 2020 · 12 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. kind/process Process oriented issues, like setting up CI priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.

Comments

@medyagh
Copy link
Member

medyagh commented Dec 12, 2020

for more information ask @priyawadhwa

@medyagh medyagh added kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. kind/process Process oriented issues, like setting up CI labels Dec 12, 2020
@Aut0R3V
Copy link
Contributor

Aut0R3V commented Dec 12, 2020

@priyawadhwa can you please explain about this issue

@priyawadhwa
Copy link

priyawadhwa commented Dec 14, 2020

Hey @Aut0R3V -- we have a binary called mkcmp in our repo -- code can be found here:

  1. https://github.com/kubernetes/minikube/blob/master/cmd/performance/mkcmp/cmd/mkcmp.go
  2. https://github.com/kubernetes/minikube/tree/master/pkg/minikube/perf

mkcmp is responsible for comparing start time between two minikube binaries. It runs each binary three times and outputs information around how long it took. This output is pasted on our PRs by our PR bot (example)

You can build the binary locally by running:

make out/mkcmp

Right now, we split up how long minikube start takes by log, the code is in this file

Instead of timing by logs to STDOUT, it would be better if we used the JSON output feature in minikube to more accurately parse logs and match up how long different steps take.

This would require running minikube start in mkcmp with the --output=json flag and parsing the output correctly.

@Aut0R3V
Copy link
Contributor

Aut0R3V commented Dec 15, 2020

Thanks a lot for the detailed explanation!

@Aut0R3V
Copy link
Contributor

Aut0R3V commented Dec 15, 2020

/assign

@priyawadhwa
Copy link

@Aut0R3V thanks for taking this issue on!

@Aut0R3V
Copy link
Contributor

Aut0R3V commented Dec 16, 2020

@priyawadhwa I had some questions :

1] What changes are to be added at the already existing mkcmp workflow ?
[https://github.com/kubernetes/minikube/blob/master/cmd/performance/mkcmp/cmd/mkcmp.go]

2] Is this the correct defaultTimeFormat for the output :
type Status struct {
defaultStatusFormat = `host: {{.Host}}

kubelet: {{.Kubelet}}

apiserver: {{.APIServer}}

kubeconfig: {{.Kubeconfig}}
`
)
If not then
What would be the defaultTimeFormat for the output ?

@priyawadhwa
Copy link

Hey @Aut0R3V --

  1. The code that needs to be changed is in this function:
    func timeCommandLogs(cmd *exec.Cmd) (*result, error) {

Basically, instead of streaming logs from STDOUT and timing how long each of them takes, we want to swtich to streaming JSON logs. JSON log steps have the following format:

{"data":{"currentstep":"0","message":"minikube v1.12.1 on Darwin 10.14.6\n","name":"Initial Minikube Setup","totalsteps":"10"},"datacontenttype":"application/json","id":"68ff70ae-202b-4b13-8351-e9f060e8c56e","source":"https://minikube.sigs.k8s.io/","specversion":"1.0","type":"io.k8s.sigs.minikube.step"}

We want to parse out the "name" of the step from the JSON output and use that, instead of the full text output. That would mean replacing this line:

log := scanner.Text()

with

log = <name from parsed JSON output>
  1. I'm not sure what your question is, could you clarify?

This issue is definitely complicated, so thanks for your interest!

@Aut0R3V
Copy link
Contributor

Aut0R3V commented Dec 17, 2020

Thanks for your help!

@Aut0R3V
Copy link
Contributor

Aut0R3V commented Jan 8, 2021

@priyawadhwa is there any specific file where the logs would be coming to?

@priyawadhwa
Copy link

Hey @Aut0R3V we get the logs by reading STDOUT from this command as it is running:

if err := cmd.Start(); err != nil {

@prezha
Copy link
Collaborator

prezha commented Mar 10, 2021

@Aut0R3V, we haven't heard back from you for some time, are you still working on this issue?

@spowelljr spowelljr added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Apr 21, 2021
@medyagh
Copy link
Member Author

medyagh commented May 3, 2021

not needed anmore

@medyagh medyagh closed this as completed May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. kind/process Process oriented issues, like setting up CI priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Development

No branches or pull requests

5 participants