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

Telemetry does not catch all errors due to abrupt os.Exit calls #4999

Closed
valaparthvi opened this issue Aug 18, 2021 · 4 comments · Fixed by #5087
Closed

Telemetry does not catch all errors due to abrupt os.Exit calls #4999

valaparthvi opened this issue Aug 18, 2021 · 4 comments · Fixed by #5087
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@valaparthvi
Copy link
Contributor

/kind bug

What versions of software are you using?

Operating System:
Fedora 34
Output of odo version:
odo v2.2.4 (0c0aaa5)

How did you run odo exactly?

  1. odo delete out of a context directory.
  2. The above command will fail.

Actual behavior

No telemetry data received on Segment, because at one point during this command execution, LogErrorAndExit is called, which exits before the telemetry data request can be triggered.

I noticed this with odo create or basically any command where you don't have the permission to create comp in a project.

On tracking down, it seems that this is the case with all the command execution where LogErrorAndExit is called at some point, except in runnable.go.
See - https://github.com/openshift/odo/search?q=LogErrorAndExit

Expected behavior

Telemetry data must be sent for these errors.

Any logs, error output, etc?

I can think of 2 solutions, atm:

  1. We could either practice a strict behaviour where LogErrorAndExit can only be called by the runnable.go:GenericRun
  2. Or, we could put the Telemetry data trigger request inside LogErrorAndExit, which would mean modifying LogErrorAndExit and all of its usages to be able to pass data that is required for triggering the request.
@openshift-ci openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 18, 2021
@valaparthvi
Copy link
Contributor Author

Creating a component in an already existing component directory is another example.

@dharmit
Copy link
Member

dharmit commented Aug 26, 2021

IMO, this issue is more about getting rid of abrupt os.Exit calls than Telemetry itself.

@dharmit dharmit changed the title Telemetry does not catch all errors Telemetry does not catch all errors due to abrupt os.Exit calls Aug 26, 2021
@anandrkskd
Copy link
Contributor

In function LogErrorAndExit() we are using os.Exit(1) call, And this function is used in multiple places. So do we want to remove os.Exit from this fuction too?

@dharmit
Copy link
Member

dharmit commented Sep 22, 2021

In function LogErrorAndExit() we are using os.Exit(1) call, And this function is used in multiple places. So do we want to remove os.Exit from this fuction too?

That's actually the right place for os.Exit(1) to exist. If you see, it's getting called in the runnable.go many a times: https://github.com/openshift/odo/blob/e347c7ac0e22ca370ab5df74cd56f4830cb885c9/pkg/odo/genericclioptions/runnable.go#L81-L99

Try to figure out calls to os.Exit in the pkg/odo/cli layer first. And also pkg/ layer but sans pkg/odo/genericclioptions.

Once you have those calls, try to figure out if they should be there or should they be returning errors instead so that we could get data in telemetry. I think most such calls could be replaced by returning an error instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants