8000 telemetry: add no-op functionality to SegmentClient to make sure errors in initialization are not fatal by redbeam · Pull Request #4100 · crc-org/crc · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

telemetry: add no-op functionality to SegmentClient to make sure errors in initialization are not fatal #4100

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

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

redbeam
Copy link
Contributor
@redbeam redbeam commented Apr 3, 2024

Fixes: Issue #4085

A new field has been added to Client struct signalling whether the client has been initialized properly. Default value is false and it is set to true only if the initialization went as expected.
Methods on Client check if this field is true before progressing further.

In case of any error during initialization, an empty Client is returned (and its methods do nothing).

@openshift-ci openshift-ci bot requested review from cfergeau and evidolob April 3, 2024 09:46
Copy link
Contributor
@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

Looks good to me, just not sure about the isInitProperly name.

Copy link
Contributor
@gbraad gbraad left a comment

Choose a reason for hiding this comment

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

Would like to understand what it takes not to keep state. The NoopClient should work without specific knowledge to what happened... and if needed, a typecheck would be preferred.

@gbraad
Copy link
Contributor
gbraad commented Apr 17, 2024

I think something went wrong during rebase, as I see some stuff about snyk and the sw_vers fix. Do not believe they are pat of this PR.

However, the rest looks good to me,

Copy link
Contributor
@cfergeau cfergeau left a comment

Choose a reason for hiding this comment

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

Looks good to me, I'd squash the 2 commits though, we don't need to have the isInitProperly version of the code in the upstream git history.
The shortlog for both commits overflows on github, it's supposed to be kept short, with a longer description in the longer commit log.

telemetry: add no-op functionality

Currently, when telemetry initialization fails, this is a hard failure for the `crc` command and daemon, and they exit.
This commit handles this more gracefully, when there's a telemetry initialization failure, an empty `NoOp` `Client` instance is returned, and all `Client` methods check for this `NoOp` instance and return early if needed.

@gbraad
Copy link
Contributor
gbraad commented Apr 23, 2024

The shortlog for both commits overflows on github, it's supposed to be kept short,

About 20 to 30 characters.

Currently, when telemetry initialization fails, this is a hard failure
for the `crc` command and daemon, and they exit.
This commit handles this more gracefully, when there's a telemetry
initialization failure, an empty `Noop Client` instance is returned,
and all `Client` methods check for this `Noop` instance and return
early if needed.
@crc-org crc-org deleted a comment from openshift-ci bot Apr 28, 2024
@crc-org crc-org deleted a comment from openshift-ci bot Apr 28, 2024
@redbeam
Copy link
Contributor Author
redbeam commented Apr 28, 2024

@gbraad @cfergeau Thanks for the suggestions. Everything should be fixed now.

@praveenkumar praveenkumar merged commit ec97112 into main Apr 30, 2024
22 of 27 checks passed
@crc-org crc-org deleted a comment from openshift-ci bot May 1, 2024
@redbeam redbeam deleted the noop_client branch May 1, 2024 01:14
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.

4 participants
0