*: Refactor codebase with linters/formatters and upstream patterns #1253

saswatamcode · 2023-10-11T17:29:48Z

Lots of TODOs still pending (that will be done in separate PRs), but this does the following.

Bump prom client, and add rules to use similar upstream patterns for instrumentation
Deprecate ioutil
Make all errors consistent and move completely to github.com/efficientgo/core/errors
Add bingo and several linters/formatters
Deprecate fake.NewClient
Fix a bunch of lint problems (although many still exist within tests)

openshift-ci · 2023-10-11T17:29:55Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

saswatamcode · 2023-10-12T06:22:41Z

/test test-unit

openshift-ci · 2023-10-12T06:22:44Z

@saswatamcode: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test e2e-kind
/test images
/test ocm-ci-rbac
/test pr-image-mirror-endpoint-monitoring-operator
/test pr-image-mirror-grafana-dashboard-loader
/test pr-image-mirror-metrics-collector
/test pr-image-mirror-multicluster-observability-operator
/test pr-image-mirror-rbac-query-proxy
/test sonarcloud
/test test-e2e
/test test-unit

Use /test all to run all jobs.

In response to this:

/retest tests-unit

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.

saswatamcode · 2023-10-12T06:23:37Z

/test test-unit

saswatamcode · 2023-10-12T07:35:58Z

/retest

philipgough · 2023-10-12T09:12:55Z

collectors/metrics/cmd/metrics-collector/main.go

 	worker, err := forwarder.New(*cfg)
 	if err != nil {
-		return fmt.Errorf("failed to configure metrics collector: %v", err)
+		return errors.Wrap(err, "failed to configure metrics collector")


any particular reason we would choose to import this package to handle errors in favour of stdlib?

It's what we use for thanos upstream (or aiming to, promql-engine already uses it). This returns the stacktrace which is super useful at times. github.com/pkg/errors did similar I think, but is no longer maintained.

I also prefer using the stdlib with fmt.Errorf("%w"). Regarding the stack trace, I don't see why we would need it. I mean, if so, it is a symptom of bad error handling that is not descriptive enough in the stack.
I don't like depending on those small libs while the stdlib does the job. But it's me 😉

philipgough · 2023-10-12T09:13:27Z

cicd-scripts/install-dependencies.sh

+    curl -L https://github.com/operator-framework/operator-sdk/releases/download/${_OPERATOR_SDK_VERSION}/operator-sdk_linux_amd64 -o operator-sdk
+  elif [[ $OSTYPE == "darwin"* ]]; then
+    curl -L https://github.com/operator-framework/operator-sdk/releases/download/${_OPERATOR_SDK_VERSION}/operator-sdk_darwin_amd64 -o operator-sdk
+  fi


could probably make this a one liner with some uname command as a subshell?

Yup! Just fyi, these shellscript changes are from formatting, not explicit changes

yep, i get that, just flagging as I go but will pause on that and we can tackle bash maybe in isolation in a follow up

Future note: we need to parameterize the architecture here too. Not everyone is running amd64. Me and all other darwin users in the team are running arm64.

philipgough · 2023-10-12T09:14:23Z

collectors/metrics/pkg/forwarder/forwarder.go

 type RuleMatcher interface {
 	MatchRules() []string
 }

-func init() {


philipgough · 2023-10-12T09:15:24Z

collectors/metrics/pkg/forwarder/forwarder.go

+			Name: "federate_filtered_samples",
+			Help: "Tracks the number of samples filtered per federation",
+		}),
+		gaugeFederateErrors: promauto.With(cfg.Registry).NewGauge(prometheus.GaugeOpts{


should we add a metric to calc total also?

Yup, will add. I think there is also something else here that causes tests to fail, will take a look.

philipgough · 2023-10-12T09:16:05Z

collectors/metrics/pkg/http/client.go

@@ -1,101 +0,0 @@
-package http


was this unused?

Yeah, builds without it!

philipgough · 2023-10-12T09:20:09Z

collectors/metrics/test/integration/prereq.sh

-		# this package has a binary, so:
+  command -v kubectl
+  if [ $? -ne 0 ]; then
+    echo "=====Setup kubectl====="


maybe we could take some inspiration from https://github.com/rhobs/configuration/blob/main/Makefile#L216-L218 around handling this? seems cleaner?

Yeah, haven't gone through all the shellscripts yet. I might clean them up explicitly in another follow-up!

philipgough · 2023-10-12T09:22:58Z

collectors/metrics/test/integration/setup.sh

-	# kubectl are now installed in current dir 
-	echo -n "kubectl version" && kubectl version
+  echo "=====Setup kubectl====="
+  # kubectl required for kind


this all looks repeated? maybe we can simplify

philipgough · 2023-10-12T09:24:20Z

collectors/metrics/test/integration/setup.sh

+  echo "Current directory"
+  echo $(pwd)
+  cd ${WORKDIR}/..
+  git clone https://github.com/coreos/kube-prometheus.git


url is outdated and will cause a redirect

philipgough · 2023-10-12T09:26:48Z

collectors/metrics/test/integration/setup.sh

+  echo $(pwd)
+  cd ${WORKDIR}/..
+  git clone https://github.com/coreos/kube-prometheus.git
+  echo "Replace namespace with openshift-monitoring"


im trying to understnad whats happening here? on KinD cluster, why do we need to use openshift-monitoring namespace with prom operator? 🤔

thibaultmg · 2023-10-12T12:15:36Z

I reviewed the PR commit by commit. Thanks for having them well organised!
Changes look good to me. (did not review original code which I think we'll touch later)
I would avoid adding the new error package and bingo. But if everybody is used to them this is not a blocker at all.
Congrats for this huge cleaning!
Waiting for tests to pass

saswatamcode · 2023-10-14T16:20:26Z

Errors

I would avoid adding the new error package

I understand the reason of not wanting to depend on small package, but there are a few reasons, why I think it is generally better than the combination of stdlib fmt.Errorf and errors.New,

Wrapping errors with fmt.Errorf needs a %w which is super easy to forget to do, and easy to miss in review. I think we can construct some linter rule for it using some custom lint tools (faillint, golangci-lint don't do this I think), but that's extra check to maintain. Due to the nature of such a single arg function, it is prone to inconsistent wrapping.
Relying on errors.New for new errors and then fmt.Errorf for wrapping doesn't look clean, and increases cognitive load. I'd rather prefer using single package consistently for everything.

Regarding the stack trace, I don't see why we would need it. I mean, if so, it is a symptom of bad error handling that is not descriptive enough in the stack.
I don't like depending on those small libs while the stdlib does the job.

I get what you're saying about non-descriptive errors @thibaultmg, but I'd argue stack traces aren't exactly to describe the error or "cause" of the error, but rather to highlight the specific code path that the error resulted from, which is extremely useful when debugging issues with complex codebases like this one. And stacktraces are useful enough that people even created o11y signal from it i.e profiling. I'd rather have stacktraces and not use it, rather than not have stacktraces but need it. 😅 Wdyt?

And if anything, we maintain efficientgo/core, so there isn't much worry about governance.

Bingo

and bingo

Curious about this, why do we want to avoid bingo? Is it the extra files or something else you noticed @thibaultmg?

Logging

Also, I have an outstanding question apart from errors, which is logging. Seems like this repo uses go-kit/log (which we use upstream quite a lot in Prometheus and Thanos), and klog which is the standard k8s logger, quite a lot. Also, Kubebuilder-based operators usually use zapr which is what obo uses. I'd like to move to one consistent logger. What do you think we should use here then? My personal pick would be go-kit/log as we can do multiple formats and filter via log level, and it's the one current team is most familiar with. But what do you think @thibaultmg @philipgough? 🙂

saswatamcode · 2023-10-16T05:31:43Z

/retest

douglascamata · 2023-10-16T12:19:51Z

On error handling, I heavily agree with @thibaultmg's point.

I do not think adding a %w is easy to forget and miss in review. In fact, it becomes instinct after a few uses. Some of us in the team might see it as easy to forget only because we lack experience with the standard library's error wrapping. It's a matter of getting acquainted with the habit (less things to keep inside our heads = cognitive_load--).
I don't think fmt.Errorf implies in extra cognitive load -- where would such load come from? It might not look clean for some, which is fair, but not a good reason to rely on a 3rd party library. In fact, we are better off using the Golang's standard library way, which can be improved over time. The Go team will take care of the backwards compatibility and migration for us.
Extra: by using the official error wrapping of Go, in the future we might get stacktraces for free and won't need a 3rd party package for this. To me it's fine to use a 3rd party package for stacktraces if they prove helpful. More than once I already caught myself with a stacktrace that didn't help me pinpoint the issue.

On Bingo, I also agree with @thibaultmg's point.

It's not about "why avoid Bingo?" it's about "why use Bingo if go install is enough"? Bingo comes from a time where go install didn't exist. Nowadays we have a proper solution from the Go team which they will evolve over time together with all the rest of the toolchain. It's much more likely that a Go developer already knows go install (less things to keep inside our heads = cognitive_load--) than Bingo.
It's not only about how many files Bingo adds to the project, but also how much maintenance is required to upgrade tooling (more things to keep inside our heads = cognitive_load++). Meanwhile, go install is just a few lines in a README, Makefile or Shell script -- to upgrade something I change the version number and that's all. Also note that the commands in Bingo and go install have slightly different syntax sometimes (more things to keep inside our heads = cognitive_load++).

Overall, my opinion is: I like to stick with the standard library whenever possible. We should all be very familiar with it. When not possible, let's look for tools that are "de-facto" standards in the community. There are many good reasons for this, including but not limited to:

By using the standard library and "de-facto" standard packages, we reduce onboarding time and cost of codebase because we will be using constructs that are familiar to every Go developer (less things to keep inside our heads = cognitive_load--). On the maintainership side, we don't have to create and maintain a package for this. The less code we have to maintain (less things to keep inside our heads = cognitive_load--), the more we can focus on what matters most.
We see that the standard library is constantly evolving, getting new features and incorporating feedback from people using it. The Go team has been doing a great job at this and keeping backwards compatibility. It's a bad idea to lock ourselves out of it or push the responsibility of adopting updates to the standard library to someone else.

thibaultmg · 2023-10-16T12:52:48Z

Hey,

Relying on errors.New for new errors and then fmt.Errorf for wrapping doesn't look clean, and increases cognitive load. I'd rather prefer using single package consistently for everything.

I think it is more of a matter of what tools you are used to. Because for me, it is this errors.Wrap that increases the cognitive load. I have to check the package to understand why it's there.
Regarding the stack trace, this can be added when debugging with 3 lines of code. And it happens rarely. So adding a specific dependency for that doesn't seem worth it to me.

Curious about this, why do we want to avoid bingo? Is it the extra files or something else you noticed

I admit it brings useful things to the table. But I don't think we need it enough to not use go install. We could isolate these dev bins in a .bin dir in the package and have all tools consume these deps from here. The other useful things is the ability to selectively add deps. We could have one make target by dep. To me there are few things that bingo brings that cannot be done with standard tooling plus some small conventions.

Also, I have an outstanding question apart from errors, which is logging.

I don't have strong opinions on this. In my personal projects I use zerolog which I like. With go 1.21 I would give a shot to slog from the standard lib.

Overall I totally agree with @douglascamata remarks about the benefits of using the standard lib as much as possible. This is what I strive for in my projects. But again, It is not a blocker if the majority of the team enjoys these tools 😉

thibaultmg

Looks Good! 🚀

douglascamata

LGTM 🚀

openshift-ci · 2023-10-17T08:35:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: douglascamata, saswatamcode, thibaultmg

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [douglascamata,saswatamcode]

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

Signed-off-by: Saswata Mukherjee <[email protected]>

…h github.com/efficientgo/core/errors Signed-off-by: Saswata Mukherjee <[email protected]>

Signed-off-by: Saswata Mukherjee <[email protected]>

openshift-ci · 2023-10-17T12:36:26Z

New changes are detected. LGTM label has been removed.

saswatamcode · 2023-10-17T13:20:50Z

/retest

sonarqubecloud · 2023-10-17T13:22:56Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
13 Code Smells

41.3% Coverage
2.0% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

openshift-ci · 2023-10-17T13:23:03Z

@saswatamcode: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/sonarcloud	`a64109d`	link	true	`/test sonarcloud`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

openshift-ci bot added the do-not-merge/work-in-progress label Oct 11, 2023

openshift-ci bot added dco-signoff: yes approved labels Oct 11, 2023

saswatamcode force-pushed the lint-fmt branch from b16d92c to 200e76d Compare October 12, 2023 05:53

saswatamcode marked this pull request as ready for review October 12, 2023 06:13

openshift-ci bot removed the do-not-merge/work-in-progress label Oct 12, 2023

saswatamcode self-assigned this Oct 12, 2023

saswatamcode requested a review from subbarao-meduri October 12, 2023 06:33

philipgough reviewed Oct 12, 2023

View reviewed changes

collectors/metrics/pkg/forwarder/forwarder.go

type RuleMatcher interface {

MatchRules() []string

}

func init() {

Copy link

Contributor

philipgough Oct 12, 2023

Choose a reason for hiding this comment

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

👍

philipgough reviewed Oct 12, 2023

View reviewed changes

saswatamcode force-pushed the lint-fmt branch 2 times, most recently from c635ee9 to 81c6df5 Compare October 15, 2023 12:54

douglascamata self-requested a review October 16, 2023 11:14

openshift-ci bot assigned thibaultmg Oct 17, 2023

openshift-ci bot added the lgtm label Oct 17, 2023

thibaultmg approved these changes Oct 17, 2023

View reviewed changes

douglascamata approved these changes Oct 17, 2023

View reviewed changes

openshift-ci bot assigned douglascamata Oct 17, 2023

saswatamcode added 17 commits October 17, 2023 18:04

Add formatters/linters and init bingo

45c4dd0

Signed-off-by: Saswata Mukherjee <[email protected]>

Run formatter + bump some prom libs

912cf01

Signed-off-by: Saswata Mukherjee <[email protected]>

Deprecate ioutil

d0607af

Signed-off-by: Saswata Mukherjee <[email protected]>

Follow promauto registry pattern in collector + remove unused code

3b2088b

Signed-off-by: Saswata Mukherjee <[email protected]>

Make all errors consistent, replace errors, github.com/pkg/errors wit…

10f3731

…h github.com/efficientgo/core/errors Signed-off-by: Saswata Mukherjee <[email protected]>

Try to address initial lint errors

61f50c4

Signed-off-by: Saswata Mukherjee <[email protected]>

Address even more lint issues

8e43886

Signed-off-by: Saswata Mukherjee <[email protected]>

Remove deprecated logger

808c886

Signed-off-by: Saswata Mukherjee <[email protected]>

Remove deprecated fake.NewFakeClient

63ecc5c

Signed-off-by: Saswata Mukherjee <[email protected]>

Use exp/slices to fix test

a085327

Signed-off-by: Saswata Mukherjee <[email protected]>

Make lint fully green

76cd33a

Signed-off-by: Saswata Mukherjee <[email protected]>

Add copyright script and enable go vet

3ddbd5f

Signed-off-by: Saswata Mukherjee <[email protected]>

Add lint as a workflow to CI

e8f72ee

Signed-off-by: Saswata Mukherjee <[email protected]>

Fix Makefile and forwarder metrics/tests

af0e9fb

Signed-off-by: Saswata Mukherjee <[email protected]>

Fix build_info collector metric

fabfae4

Signed-off-by: Saswata Mukherjee <[email protected]>

Revert move to github.com/efficientgo/core/errors for stdlib

f80a405

Signed-off-by: Saswata Mukherjee <[email protected]>

Address comments

a64109d

Signed-off-by: Saswata Mukherjee <[email protected]>

saswatamcode force-pushed the lint-fmt branch from c5feb91 to a64109d Compare October 17, 2023 12:36

openshift-ci bot removed the lgtm label Oct 17, 2023

subbarao-meduri merged commit 1d6732a into stolostron:main Oct 17, 2023

*: Refactor codebase with linters/formatters and upstream patterns #1253

*: Refactor codebase with linters/formatters and upstream patterns #1253

Conversation

saswatamcode commented Oct 11, 2023 • edited Loading

openshift-ci bot commented Oct 11, 2023

saswatamcode commented Oct 12, 2023 • edited Loading

openshift-ci bot commented Oct 12, 2023

saswatamcode commented Oct 12, 2023

saswatamcode commented Oct 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultmg commented Oct 12, 2023

saswatamcode commented Oct 14, 2023 • edited Loading

Errors

Bingo

Logging

saswatamcode commented Oct 16, 2023

douglascamata commented Oct 16, 2023

thibaultmg commented Oct 16, 2023

thibaultmg left a comment

Choose a reason for hiding this comment

douglascamata left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 17, 2023

openshift-ci bot commented Oct 17, 2023

saswatamcode commented Oct 17, 2023

sonarqubecloud bot commented Oct 17, 2023

openshift-ci bot commented Oct 17, 2023 • edited Loading

saswatamcode commented Oct 11, 2023 •

edited

Loading

saswatamcode commented Oct 12, 2023 •

edited

Loading

saswatamcode commented Oct 14, 2023 •

edited

Loading

openshift-ci bot commented Oct 17, 2023 •

edited

Loading