Repository: knative/pkg
Pull request #1974 (2020-12-23 23:05:52)

Introduce a nostackdriver build tag
This teaches `depcheck` to support passing build flags through, and uses it to ensure that with `-tags=nostackdriver` that the expensive stack driver libraries are not linked.

I'm starting in `./tracing` since it is considerably simpler than the mess in metrics, but my intention is to do something similar in `./metrics` next.

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1974#" title="Author self-approved">mattmoor</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/pkg/blob/master/OWNERS)~~ [mattmoor]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/pkg/pull/1974?src=pr&el=h1) Report
> Merging [#1974](https://codecov.io/gh/knative/pkg/pull/1974?src=pr&el=desc) (e410f02) into [master](https://codecov.io/gh/knative/pkg/commit/9d0775512af8df6df7ea94b049f0da0277c59702?el=desc) (9d07755) will **increase** coverage by `0.04%`.
> The diff coverage is `40.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/pkg/pull/1974/graphs/tree.svg?width=650&height=150&src=pr&token=rlqpDnfxbO)](https://codecov.io/gh/knative/pkg/pull/1974?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1974      +/-   ##
==========================================
+ Coverage   69.05%   69.10%   +0.04%     
==========================================
  Files         209      210       +1     
  Lines        8792     8793       +1     
==========================================
+ Hits         6071     6076       +5     
+ Misses       2451     2449       -2     
+ Partials      270      268       -2     
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/1974?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [tracing/opencensus.go](https://codecov.io/gh/knative/pkg/pull/1974/diff?src=pr&el=tree#diff-dHJhY2luZy9vcGVuY2Vuc3VzLmdv) | `43.05% <0.00%> (+1.72%)` | :arrow_up: |
| [tracing/stackdriver.go](https://codecov.io/gh/knative/pkg/pull/1974/diff?src=pr&el=tree#diff-dHJhY2luZy9zdGFja2RyaXZlci5nbw==) | `0.00% <0.00%> (ø)` | |
| [depcheck/depcheck.go](https://codecov.io/gh/knative/pkg/pull/1974/diff?src=pr&el=tree#diff-ZGVwY2hlY2svZGVwY2hlY2suZ28=) | `82.45% <60.00%> (+0.31%)` | :arrow_up: |
| [test/gcs/mock/mock.go](https://codecov.io/gh/knative/pkg/pull/1974/diff?src=pr&el=tree#diff-dGVzdC9nY3MvbW9jay9tb2NrLmdv) | `92.47% <0.00%> (+2.15%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/pkg/pull/1974?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/pkg/pull/1974?src=pr&el=footer). Last update [9d07755...e410f02](https://codecov.io/gh/knative/pkg/pull/1974?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

knative-prow-robot: @mattmoor: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | e410f0271e11389e47bbde0d3df7ccb06c6c1e4e | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/1974/pull-knative-pkg-unit-tests/1341882771873730560) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1974). [Your PR dashboard](https://gubernator.knative.dev/pr/mattmoor).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
mattmoor: /retest
knative-prow-robot: @mattmoor: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | e410f0271e11389e47bbde0d3df7ccb06c6c1e4e | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/1974/pull-knative-pkg-unit-tests/1341884083558420480) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1974). [Your PR dashboard](https://gubernator.knative.dev/pr/mattmoor).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
mattmoor: /retest

Patch:
diff --git a/.github/workflows/nostackdriver.yaml b/.github/workflows/nostackdriver.yaml
new file mode 100644
index 000000000..d1c80de2c
--- /dev/null
+++ b/.github/workflows/nostackdriver.yaml
@@ -0,0 +1,37 @@
+# Copyright 2020 The Knative Authors.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+name: Test
+
+on:
+  pull_request:
+    branches: [ 'main', 'master', 'release-*' ]
+
+jobs:
+
+  nostackdriver:
+    name: Unit Tests w/o stackdriver
+    runs-on: ubuntu-latest
+
+    steps:
+      - name: Set up Go 1.15.x
+        uses: actions/setup-go@v2
+        with:
+          go-version: 1.15.x
+
+      - name: Check out code
+        uses: actions/checkout@v2
+
+      - name: Test w/nostackdriver
+        run: go test -tags=nostackdriver -race ./...
diff --git a/depcheck/depcheck.go b/depcheck/depcheck.go
index 14d5c6674..034617451 100644
--- a/depcheck/depcheck.go
+++ b/depcheck/depcheck.go
@@ -75,11 +75,12 @@ func (g graph) path(name string) []string {
 	return append(base, name)
 }
 
-func buildGraph(importpaths ...string) (graph, error) {
-	g := make(graph, len(importpaths))
+func buildGraph(importpath string, buildFlags ...string) (graph, error) {
+	g := make(graph, 1)
 	pkgs, err := packages.Load(&packages.Config{
-		Mode: packages.NeedName | packages.NeedFiles | packages.NeedCompiledGoFiles | packages.NeedImports | packages.NeedDeps | packages.NeedModule,
-	}, importpaths...)
+		Mode:       packages.NeedName | packages.NeedFiles | packages.NeedCompiledGoFiles | packages.NeedImports | packages.NeedDeps | packages.NeedModule,
+		BuildFlags: buildFlags,
+	}, importpath)
 	if err != nil {
 		return nil, err
 	}
@@ -101,10 +102,10 @@ func buildGraph(importpaths ...string) (graph, error) {
 
 // CheckNoDependency checks that the given import paths (ip) does not
 // depend (transitively) on certain banned imports.
-func CheckNoDependency(ip string, banned []string) error {
-	g, err := buildGraph(ip)
+func CheckNoDependency(ip string, banned []string, buildFlags ...string) error {
+	g, err := buildGraph(ip, buildFlags...)
 	if err != nil {
-		return fmt.Errorf("buildGraph(queue) = %w", err)
+		return fmt.Errorf("buildGraph(%q) = %w", ip, err)
 	}
 	for _, dip := range banned {
 		if g.contains(dip) {
@@ -117,11 +118,11 @@ func CheckNoDependency(ip string, banned []string) error {
 
 // AssertNoDependency checks that the given import paths (the keys) do not
 // depend (transitively) on certain banned imports (the values)
-func AssertNoDependency(t *testing.T, banned map[string][]string) {
+func AssertNoDependency(t *testing.T, banned map[string][]string, buildFlags ...string) {
 	t.Helper()
 	for ip, banned := range banned {
 		t.Run(ip, func(t *testing.T) {
-			if err := CheckNoDependency(ip, banned); err != nil {
+			if err := CheckNoDependency(ip, banned, buildFlags...); err != nil {
 				t.Error("CheckNoDependency() =", err)
 			}
 		})
@@ -133,10 +134,10 @@ func AssertNoDependency(t *testing.T, banned map[string][]string) {
 // Note: while perhaps counterintuitive we allow the value to be a superset
 // of the actual imports to that folks can use a constant that holds blessed
 // import paths.
-func CheckOnlyDependencies(ip string, allowed map[string]struct{}) error {
-	g, err := buildGraph(ip)
+func CheckOnlyDependencies(ip string, allowed map[string]struct{}, buildFlags ...string) error {
+	g, err := buildGraph(ip, buildFlags...)
 	if err != nil {
-		return fmt.Errorf("buildGraph(queue) = %w", err)
+		return fmt.Errorf("buildGraph(%q) = %w", ip, err)
 	}
 	for _, name := range g.order() {
 		if _, ok := allowed[name]; !ok {
@@ -152,7 +153,7 @@ func CheckOnlyDependencies(ip string, allowed map[string]struct{}) error {
 // Note: while perhaps counterintuitive we allow the value to be a superset
 // of the actual imports to that folks can use a constant that holds blessed
 // import paths.
-func AssertOnlyDependencies(t *testing.T, allowed map[string][]string) {
+func AssertOnlyDependencies(t *testing.T, allowed map[string][]string, buildFlags ...string) {
 	t.Helper()
 	for ip, allow := range allowed {
 		// Always include our own package in the set of allowed dependencies.
@@ -161,7 +162,7 @@ func AssertOnlyDependencies(t *testing.T, allowed map[string][]string) {
 			allowed[x] = struct{}{}
 		}
 		t.Run(ip, func(t *testing.T) {
-			if err := CheckOnlyDependencies(ip, allowed); err != nil {
+			if err := CheckOnlyDependencies(ip, allowed, buildFlags...); err != nil {
 				t.Error("CheckOnlyDependencies() =", err)
 			}
 		})
diff --git a/tracing/nostackdriver.go b/tracing/nostackdriver.go
new file mode 100644
index 000000000..f504c1ffd
--- /dev/null
+++ b/tracing/nostackdriver.go
@@ -0,0 +1,30 @@
+// +build nostackdriver
+
+/*
+Copyright 2020 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package tracing
+
+import (
+	"errors"
+
+	"go.opencensus.io/trace"
+	"knative.dev/pkg/tracing/config"
+)
+
+func newStackdriver(cfg *config.Config) (trace.Exporter, error) {
+	return nil, errors.New("Stackdriver is not configured")
+}
diff --git a/tracing/opencensus.go b/tracing/opencensus.go
index 4346593ba..fbe27c048 100644
--- a/tracing/opencensus.go
+++ b/tracing/opencensus.go
@@ -1,3 +1,19 @@
+/*
+Copyright 2020 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
 package tracing
 
 import (
@@ -7,9 +23,8 @@ import (
 	"os"
 	"sync"
 
-	"contrib.go.opencensus.io/exporter/stackdriver"
 	oczipkin "contrib.go.opencensus.io/exporter/zipkin"
-	zipkin "github.com/openzipkin/zipkin-go"
+	"github.com/openzipkin/zipkin-go"
 	httpreporter "github.com/openzipkin/zipkin-go/reporter/http"
 	"go.opencensus.io/trace"
 	"go.uber.org/zap"
@@ -128,17 +143,14 @@ func WithExporterFull(name, host string, logger *zap.SugaredLogger) ConfigOption
 		var (
 			exporter trace.Exporter
 			closer   io.Closer
+			err      error
 		)
 		switch cfg.Backend {
 		case config.Stackdriver:
-			exp, err := stackdriver.NewExporter(stackdriver.Options{
-				ProjectID: cfg.StackdriverProjectID,
-			})
+			exporter, err = newStackdriver(cfg)
 			if err != nil {
-				logger.Errorw("error reading project-id from metadata", zap.Error(err))
 				return err
 			}
-			exporter = exp
 		case config.Zipkin:
 			// If host isn't specified, then zipkin.NewEndpoint will return an error saying that it
 			// can't find the host named ''. So, if not specified, default it to this machine's
@@ -161,6 +173,7 @@ func WithExporterFull(name, host string, logger *zap.SugaredLogger) ConfigOption
 			reporter := httpreporter.NewReporter(cfg.ZipkinEndpoint)
 			exporter = oczipkin.NewExporter(reporter, zipEP)
 			closer = reporter
+
 		default:
 			// Disables tracing.
 		}
diff --git a/tracing/stackdriver.go b/tracing/stackdriver.go
new file mode 100644
index 000000000..b8986a2a7
--- /dev/null
+++ b/tracing/stackdriver.go
@@ -0,0 +1,31 @@
+// +build !nostackdriver
+
+/*
+Copyright 2020 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package tracing
+
+import (
+	"contrib.go.opencensus.io/exporter/stackdriver"
+	"go.opencensus.io/trace"
+	"knative.dev/pkg/tracing/config"
+)
+
+func newStackdriver(cfg *config.Config) (trace.Exporter, error) {
+	return stackdriver.NewExporter(stackdriver.Options{
+		ProjectID: cfg.StackdriverProjectID,
+	})
+}


Test patch:
diff --git a/tracing/depcheck_test.go b/tracing/depcheck_test.go
new file mode 100644
index 000000000..875ac955f
--- /dev/null
+++ b/tracing/depcheck_test.go
@@ -0,0 +1,29 @@
+/*
+Copyright 2020 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package tracing_test
+
+import (
+	"testing"
+
+	"knative.dev/pkg/depcheck"
+)
+
+func TestNoDeps(t *testing.T) {
+	depcheck.AssertNoDependency(t, map[string][]string{
+		"knative.dev/pkg/tracing": depcheck.KnownHeavyDependencies,
+	}, "-tags=nostackdriver")
+}