drop stackdriver metrics and tracing exporters #2183

dprotaso · 2021-07-08T14:35:03Z

drop stackdriver as a tracing backend
drop stackdriver as a metrics backend
update deps - dropping stackdriver

codecov · 2021-07-08T14:39:27Z

Codecov Report

Merging #2183 (d85ea54) into main (51cfaab) will increase coverage by 0.06%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main    #2183      +/-   ##
==========================================
+ Coverage   66.05%   66.11%   +0.06%     
==========================================
  Files         224      221       -3     
  Lines        9426     9259     -167     
==========================================
- Hits         6226     6122     -104     
+ Misses       2920     2862      -58     
+ Partials      280      275       -5

Impacted Files	Coverage Δ
metrics/config_observability.go	`92.00% <ø> (ø)`
source/source_stats_reporter.go	`84.90% <ø> (ø)`
test/spoof/error_checks.go	`76.92% <0.00%> (-6.42%)`	⬇️
test/spoof/spoof.go	`48.43% <0.00%> (+48.43%)`	⬆️
tracing/opencensus.go	`46.26% <ø> (+3.21%)`	⬆️
metrics/config.go	`77.77% <100.00%> (-4.47%)`	⬇️
metrics/exporter.go	`84.09% <100.00%> (-0.53%)`	⬇️
tracing/config/tracing.go	`80.76% <100.00%> (+5.76%)`	⬆️
metrics/resource_view.go	`86.93% <0.00%> (-1.71%)`	⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe90576...d85ea54. Read the comment docs.

dprotaso · 2021-07-08T14:40:01Z

/hold - going to wait till next week in case there's some commentary on the issue

/assign @evankanderson @skonto

dprotaso · 2021-07-12T14:19:50Z

/hold cancel

skonto · 2021-07-12T14:40:39Z

metrics/config_test.go

-	customSubDomain        = "test.domain"
-	testProj               = "test-project"
-	anotherProj            = "another-project"
+	metricsDomain = "knative.dev/project"


What was the value of having different domains?

This was a stack driver thing but we're actually prefixing this to the open census metric names so I think this should keep this around until we resolve: #2174

I think there's some value in repurposing this as a label in order to help disambiguate between components across different Knative projects (ie. serving vs eventing).

ie. in the future we drop the metric prefix and thus require labels to help segment metrics.

So for a golang memory metric - we'd have a component label ie. controller and reuse the domain label serving

I'm ok to drop this domain if an alternative exists - ie. if the components namespace is automatically picked up as part of the metric

I think the namespace is likely to be picked up as part of the metric; I'm not sure if we have to / want to worry about cases like queue-proxies and sources running in a user's namespace.

I'm inclined to think that namespace + some sort of component/Resource name should be sufficient here, but I might be mistaken.

(With the notion that the Resource would indicate whether it was a serving or eventing construct if it was running in a user namespace, and that multi-tenant components would be running in a namespace where the name would give a clue.)

The obvious counter-example here is mink, where all the controllers are bundled into a single Pod, but I'm not sure that's a compelling enough case.

skonto · 2021-07-12T14:50:30Z

@dprotaso I still see some references to the stackdriver name like here:

pkg/logging/config.go

Line 58 in 952ee7d

loggingCfg := stackdriverConfig()

Shouldn't we remove it from the whole codebase? There are more from a quick search, detected via the IDE.

dprotaso · 2021-07-12T15:35:43Z

re:

loggingCfg := stackdriverConfig()

This just produces a logging format that stackdriver can parse easily - I'm ok with leaving this since it helps debugging with our CI/prow runs.

Do you have suggestions on other alternatives here?

skonto · 2021-07-12T15:46:28Z

Do you have suggestions on other alternatives here?

I can do a followup PR to cleanup anything that remains and discuss them in detail one by one. For that particular logging config func I would just rename it, if it does not do any harm.

dprotaso · 2021-07-12T21:32:13Z

/hold

I'll fix the downstream serving issues

dprotaso · 2021-07-12T23:09:50Z

Downstream issue should be resolved here:
knative/serving#11659

dprotaso · 2021-07-13T00:47:08Z

Moving Eventing Source StatReporter out of knative.dev/pkg - knative/eventing#5587

these have been moved to their respective repos see: knative#608

We're going to move the StatsReporter downstream eventually related: knative/eventing#5587 knative/eventing#5586

dprotaso · 2021-07-14T12:57:22Z

I'm going to temporarily move the eventing sources metric keys to pkg/source until we can move the stats reporter into the eventing repo. That's sometime I'm working on now

dprotaso · 2021-07-14T13:21:57Z

last PR to fix serving downstream: knative/serving#11679

dprotaso · 2021-07-14T15:55:18Z

/hold cancel - this is good now

dprotaso · 2021-07-14T16:44:54Z

/hold cancel

evankanderson

/lgtm
/approve

I'm amused that we're deleting about 10k lines of aws-sdk-go with this change. (Also, wow the generated proto files stackdriver pulled in were big...)

evankanderson · 2021-07-14T18:26:31Z

metrics/config_observability.go

-	// RequestMetricsBackend specifies the request metrics destination, e.g. Prometheus,
-	// Stackdriver. "None" disables all backends.
+	// RequestMetricsBackend specifies the request metrics destination, e.g. Prometheus.
+	// "None" disables all backends.


Should this have OpenCensus?

I'll add it back

evankanderson · 2021-07-14T18:28:39Z

metrics/config_test.go

-	customSubDomain        = "test.domain"
-	testProj               = "test-project"
-	anotherProj            = "another-project"
+	metricsDomain = "knative.dev/project"


I think the namespace is likely to be picked up as part of the metric; I'm not sure if we have to / want to worry about cases like queue-proxies and sources running in a user's namespace.

I'm inclined to think that namespace + some sort of component/Resource name should be sufficient here, but I might be mistaken.

evankanderson · 2021-07-14T18:30:05Z

metrics/config_test.go

-	customSubDomain        = "test.domain"
-	testProj               = "test-project"
-	anotherProj            = "another-project"
+	metricsDomain = "knative.dev/project"


(With the notion that the Resource would indicate whether it was a serving or eventing construct if it was running in a user namespace, and that multi-tenant components would be running in a namespace where the name would give a clue.)

The obvious counter-example here is mink, where all the controllers are bundled into a single Pod, but I'm not sure that's a compelling enough case.

metrics/config_test.go

knative-prow-robot · 2021-07-14T18:45:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, evankanderson

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~~ [dprotaso,evankanderson]

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

dprotaso added 3 commits July 7, 2021 17:39

drop stack driver as a tracing backend

eac7ee7

drop stackdriver as a metrics backend

fedc32d

update deps - dropping stackdriver

544e051

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 8, 2021

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 8, 2021

knative-prow-robot assigned evankanderson and skonto Jul 8, 2021

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 8, 2021

fix linting issues

952ee7d

dprotaso force-pushed the no-stack-driver branch from df7924a to 952ee7d Compare July 8, 2021 14:55

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 12, 2021

skonto reviewed Jul 12, 2021

View reviewed changes

dprotaso mentioned this pull request Jul 12, 2021

Should our default log output be specific to stackdriver? #2187

Closed

drop further references to stack driver

d80b50d

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 12, 2021

dprotaso mentioned this pull request Jul 12, 2021

drop stackdriver exporter support knative/serving#11659

Merged

dprotaso added 2 commits July 14, 2021 08:51

drop serving & eventing metric key constants

e3d84b3

these have been moved to their respective repos see: knative#608

move source metrickeys to pkg/source

2aa3e08

We're going to move the StatsReporter downstream eventually related: knative/eventing#5587 knative/eventing#5586

dprotaso force-pushed the no-stack-driver branch from 526efe3 to 2aa3e08 Compare July 14, 2021 12:56

fix linter

d85ea54

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 14, 2021

evankanderson reviewed Jul 14, 2021

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 14, 2021

knative-prow-robot merged commit aff7376 into knative:main Jul 14, 2021

dprotaso deleted the no-stack-driver branch July 14, 2021 18:46

lionelvillard mentioned this pull request Nov 8, 2021

Remove unsupported stackdriver tracing backend knative/eventing#5881

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drop stackdriver metrics and tracing exporters #2183

drop stackdriver metrics and tracing exporters #2183

dprotaso commented Jul 8, 2021 •

edited

Loading

codecov bot commented Jul 8, 2021 •

edited

Loading

dprotaso commented Jul 8, 2021

dprotaso commented Jul 12, 2021

skonto Jul 12, 2021

dprotaso Jul 12, 2021

evankanderson Jul 14, 2021

evankanderson Jul 14, 2021

skonto commented Jul 12, 2021 •

edited

Loading

dprotaso commented Jul 12, 2021

skonto commented Jul 12, 2021 •

edited

Loading

dprotaso commented Jul 12, 2021

dprotaso commented Jul 12, 2021

dprotaso commented Jul 13, 2021

dprotaso commented Jul 14, 2021 •

edited

Loading

dprotaso commented Jul 14, 2021

dprotaso commented Jul 14, 2021

dprotaso commented Jul 14, 2021

evankanderson left a comment

evankanderson Jul 14, 2021

dprotaso Jul 14, 2021

evankanderson Jul 14, 2021

evankanderson Jul 14, 2021

knative-prow-robot commented Jul 14, 2021

drop stackdriver metrics and tracing exporters #2183

drop stackdriver metrics and tracing exporters #2183

Conversation

dprotaso commented Jul 8, 2021 • edited Loading

codecov bot commented Jul 8, 2021 • edited Loading

Codecov Report

dprotaso commented Jul 8, 2021

dprotaso commented Jul 12, 2021

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

skonto commented Jul 12, 2021 • edited Loading

dprotaso commented Jul 12, 2021

skonto commented Jul 12, 2021 • edited Loading

dprotaso commented Jul 12, 2021

dprotaso commented Jul 12, 2021

dprotaso commented Jul 13, 2021

dprotaso commented Jul 14, 2021 • edited Loading

dprotaso commented Jul 14, 2021

dprotaso commented Jul 14, 2021

dprotaso commented Jul 14, 2021

evankanderson left a 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

knative-prow-robot commented Jul 14, 2021

dprotaso commented Jul 8, 2021 •

edited

Loading

codecov bot commented Jul 8, 2021 •

edited

Loading

skonto commented Jul 12, 2021 •

edited

Loading

skonto commented Jul 12, 2021 •

edited

Loading

dprotaso commented Jul 14, 2021 •

edited

Loading