monitoring: migrate from OpenCensus to OpenTelemetry #45341

howardjohn · 2023-06-08T00:41:44Z

~~Blocked by open-telemetry/opentelemetry-go#4306. I have a hold until that is ready; should be fairly quick~~

This PR migrates from OpenCensus to OpenTelemetry for Go metrics. This
doesn't impact proxy metrics, or non-metrics observability.

These changes are intended to be 100% transparent to end users.

The changes are mostly transparent from pkg/monitoring public
interface, with a few changes:

Register is no longer needed. Creating the metric registers it. This
makes no real difference in any of our usage
MustCreateLabel -> CreateLabel; it is infallable
No need to define the set of keys upfront with the metric.
RegisterIf -> WithEnabled
monitortest behaves slightly different; only metrics recorded after
its creation are counted. A few tests are updated to handle this
change.
Only one exporter can be defined; this only impacts tests, which are
updated to handle this.

There are a number of changes, of course, to the internals. Most of this
is a fairly mechanical translation. A few quirks:

There is no real Gauge like in OC. Instead, we can only do a callback.
So we need to handle a bit of this on our end; not a huge deal, and
looks like it will be added eventually.
Setting histogram buckets is a bit awkward currently. This will be
addressed eventually upstream.
monitortest previously was based on OC internals. This has been re-written to prometheus instead of OTEL. This is because prometheus is our primary usage, so its a bit more realistic.

~~This results in a 1% increase in the binary size, which is due to importing both OC and Otel. This is addressed in #46010, which introduces a 1% decrease in binary size when combined with this PR.~~

Recording metrics is now much cheaper. This may allow us to start enabling metrics that were previously disabled (TBD, I am planning a more thorough design of our overall observability, would prefer to avoid changes until then):

name                      old time/op    new time/op    delta
Counter/no_labels-7          295ns ± 3%      56ns ±10%   -80.91%  (p=0.008 n=5+5)
Counter/dynamic_labels-7     870ns ±26%     434ns ± 4%   -50.16%  (p=0.008 n=5+5)
Counter/static_labels-7      407ns ± 2%     122ns ± 1%   -69.98%  (p=0.029 n=4+4)

name                      old alloc/op   new alloc/op   delta
Counter/no_labels-7          68.0B ± 0%      0.0B       -100.00%  (p=0.008 n=5+5)
Counter/dynamic_labels-7      760B ± 0%      320B ± 0%      ~     (p=0.079 n=4+5)
Counter/static_labels-7      80.0B ± 0%      0.0B       -100.00%  (p=0.008 n=5+5)

name                      old allocs/op  new allocs/op  delta
Counter/no_labels-7           3.00 ± 0%      0.00       -100.00%  (p=0.008 n=5+5)
Counter/dynamic_labels-7      13.0 ± 0%       6.0 ± 0%   -53.85%  (p=0.008 n=5+5)
Counter/static_labels-7       3.00 ± 0%      0.00       -100.00%  (p=0.008 n=5+5)

istio-testing · 2023-06-08T00:41:47Z

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

zirain · 2023-06-08T02:56:09Z

FYI: I test with opentelemetry-go offline, it's will add _total suffix fro counter, which maybe a regression

https://github.com/open-telemetry/opentelemetry-go/blob/4d473d105a5f59a58d153e7c492a4ea0ee027871/exporters/prometheus/exporter.go#L216

pkg/monitoring/monitoring.go

Currently this isn't really a big difference, just a minor refactoring to use best practices and avoid global/init(). In future (istio#45341) it may be more important.

* agent: avoid global metrics initialization Currently this isn't really a big difference, just a minor refactoring to use best practices and avoid global/init(). In future (#45341) it may be more important. * fix tests

zirain

Great work! Thanks @howardjohn

zirain · 2023-07-16T01:06:00Z

security/pkg/nodeagent/plugin/providers/google/stsclient/stsclient_test.go

@@ -75,6 +75,7 @@ func TestGetFederatedToken(t *testing.T) {
 		tag := map[string]string{
 			"request_type": monitoring.TokenExchange,
 		}
+		// TODO: this is broken due to prometheus auto adding "_total"
 		mt.Assert("num_outgoing_retries", tag, monitortest.AtLeast(1))


is this worth a release notes?

Stale comment - its not broken anymore. Let me clean this up

For istio#44743 Fixes istio#44513 This PR migrates from OpenCensus to OpenTelemetry for Go metrics. This doesn't impact proxy metrics, or non-metrics observability. These changes are intended to be 100% transparent to end users. The changes are *mostly* transparent from `pkg/monitoring` public interface, with a few changes: * `Register` is no longer needed. Creating the metric registers it. This makes no real difference in any of our usage * MustCreateLabel -> CreateLabel; it is infallable * No need to define the set of keys upfront with the metric. * RegisterIf -> WithEnabled * monitortest behaves slightly different; only metrics recorded after its creation are counted. A few tests are updated to handle this change. * Only one exporter can be defined; this only impacts tests, which are updated to handle this. There are a number of changes, of course, to the internals. Most of this is a fairly mechanical translation. A few quirks: * There is no real Gauge like in OC. Instead, we can only do a callback. So we need to handle a bit of this on our end; not a huge deal, and looks like it will be added eventually. * Setting histogram buckets is a bit awkward currently. This will be addressed eventually upstream.

costinm · 2023-07-17T17:51:50Z

Looks very good.

One concern - not blocking this PR - is that I would prefer if labels are declared at metric creation, even if otel doesn't require it.

costinm

I am approving the PR - not entirely happy, but it is clearly a huge improvement (opencensus is long deprecated and unsupported).

I think we need to follow up - and remove the wrappers, if we use otel let's use the otel API without an istio abstraction that just confuses people. And add back some info about the labels for each metris - in the description or somewhere, we also probably need to generate docs for each metric.

All of this can be done in separate PRs and incrementally.

I would also add some doc - and make it clear if indeed the intent is to start using otel API and if it is ok for new metrics ( and gradually for the old ones ) to replace the use of the wrapper with the proper API.

costinm · 2023-07-17T19:27:23Z

cni/pkg/install/monitoring.go

@@ -26,16 +26,10 @@ var (
 	cniInstalls = monitoring.NewSum(
 		"istio_cni_installs_total",
 		"Total number of CNI plugins installed by the Istio CNI installer",
-		monitoring.WithLabels(resultLabel),


I would not remove this part ( maybe refactor it to avoid the MustCreateLabel - but having a list of labels is very informative, and will still be required if we want monitoring to have a prometheus implementation.

If we want OTel only ( which I don't mind ) - we don't need an abstraction/wrapper on top of the SDK. But even then it would be good to keep track of what labels are generated, it is probably the most critical aspect of a metric.

costinm · 2023-07-17T19:29:33Z

operator/pkg/metrics/monitoring.go


 	// ReconcileRequestReasonLabel describes reason of reconcile request.
-	ReconcileRequestReasonLabel = monitoring.MustCreateLabel("reason")
+	ReconcileRequestReasonLabel = monitoring.CreateLabel("reason")


Where are they used - I see WithLabels is removed.

costinm · 2023-07-17T19:30:07Z

operator/pkg/metrics/monitoring.go

-
-		ReconcileRequestTotal,
-	)
-
 	initOperatorCrdResourceMetrics()


What does it do - isn't it another must register ?

costinm · 2023-07-17T19:31:06Z

pilot/cmd/pilot-agent/status/server.go

 }

-func initializeMonitoring() (*prometheus.Registry, error) {
+func initializeMonitoring() (prometheus.Gatherer, error) {


If we're doing this - shouldn't agent also use otel sdk ? Not in this PR, of course - but a TODO and comment explaining why prometheus is used directly would help other developers.

costinm · 2023-07-17T19:33:45Z

pilot/pkg/config/kube/crdclient/metrics.go

@@ -19,20 +19,15 @@ import (
 )

 var (
-	typeTag  = monitoring.MustCreateLabel("type")
-	eventTag = monitoring.MustCreateLabel("event")
+	typeTag  = monitoring.CreateLabel("type")


Not entirely sure why this was renamed - either remove or leave it as is, we can live with 'Must' for a while...

howardjohn added the release-notes-none Indicates a PR that does not require release notes. label Jun 8, 2023

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jun 8, 2023

istio-testing added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 8, 2023

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jun 11, 2023

zirain mentioned this pull request Jun 13, 2023

cleanup pkg/monitoring #45271

Closed

zirain reviewed Jun 14, 2023

View reviewed changes

pkg/monitoring/monitoring.go Outdated Show resolved Hide resolved

howardjohn force-pushed the exp/otel-sdk branch from 134270b to eb4c3d0 Compare June 30, 2023 00:52

howardjohn force-pushed the exp/otel-sdk branch from eb4c3d0 to ccedd68 Compare July 10, 2023 15:49

istio-testing added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR needs to be rebased before being merged size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 10, 2023

howardjohn mentioned this pull request Jul 12, 2023

agent: avoid global metrics initialization #45993

Merged

howardjohn force-pushed the exp/otel-sdk branch from ba95082 to 5068c09 Compare July 13, 2023 21:36

howardjohn changed the title ~~wip: Swap out for otel SDK~~ monitoring: migrate from OpenCensus to OpenTelemetry Jul 13, 2023

howardjohn force-pushed the exp/otel-sdk branch from 5068c09 to a2e551c Compare July 13, 2023 21:51

howardjohn added the do-not-merge/hold Block automatic merging of a PR. label Jul 13, 2023

howardjohn marked this pull request as ready for review July 13, 2023 21:51

howardjohn requested review from a team, costinm, linsun and stevenctl as code owners July 13, 2023 21:51

howardjohn requested review from ericvn and hzxuzhonghu as code owners July 13, 2023 21:51

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jul 13, 2023

howardjohn force-pushed the exp/otel-sdk branch 2 times, most recently from 6ecac3b to 31fc60b Compare July 14, 2023 17:04

howardjohn removed the do-not-merge/hold Block automatic merging of a PR. label Jul 14, 2023

zirain approved these changes Jul 16, 2023

View reviewed changes

ramaraochavali approved these changes Jul 17, 2023

View reviewed changes

howardjohn force-pushed the exp/otel-sdk branch from 31fc60b to ec70c29 Compare July 17, 2023 15:41

ericvn approved these changes Jul 17, 2023

View reviewed changes

costinm approved these changes Jul 17, 2023

View reviewed changes

istio-testing merged commit eb0f84c into istio:master Jul 17, 2023
27 checks passed

zirain mentioned this pull request Jul 18, 2023

Deprecation announcement: OpenCensus sunset on 7/31 #44743

Closed

ra-grover mentioned this pull request Jul 20, 2023

Queue metrics #45734

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

monitoring: migrate from OpenCensus to OpenTelemetry #45341

monitoring: migrate from OpenCensus to OpenTelemetry #45341

howardjohn commented Jun 8, 2023 •

edited

istio-testing commented Jun 8, 2023

zirain commented Jun 8, 2023

zirain left a comment

zirain Jul 16, 2023

howardjohn Jul 17, 2023

costinm commented Jul 17, 2023

costinm left a comment

costinm Jul 17, 2023

costinm Jul 17, 2023

costinm Jul 17, 2023

costinm Jul 17, 2023

costinm Jul 17, 2023

monitoring: migrate from OpenCensus to OpenTelemetry #45341

monitoring: migrate from OpenCensus to OpenTelemetry #45341

Conversation

howardjohn commented Jun 8, 2023 • edited

istio-testing commented Jun 8, 2023

zirain commented Jun 8, 2023

zirain left a comment

Choose a reason for hiding this comment

zirain Jul 16, 2023

Choose a reason for hiding this comment

howardjohn Jul 17, 2023

Choose a reason for hiding this comment

costinm commented Jul 17, 2023

costinm left a comment

Choose a reason for hiding this comment

costinm Jul 17, 2023

Choose a reason for hiding this comment

costinm Jul 17, 2023

Choose a reason for hiding this comment

costinm Jul 17, 2023

Choose a reason for hiding this comment

costinm Jul 17, 2023

Choose a reason for hiding this comment

costinm Jul 17, 2023

Choose a reason for hiding this comment

howardjohn commented Jun 8, 2023 •

edited