Separate OpenCensus and remove unused methods #804
Conversation
|
😊 Welcome @costinm! This is either your first contribution to the Istio pkg repo, or it's been You can learn more about the Istio working groups, code of conduct, and contributing guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
istio-testing
added
the
size/XL
| Name() string | ||
|
|
||
| // Record makes an observation of the provided value for the given measure. | ||
| // Majority of Istio is setting this to an int |
There was a problem hiding this comment.
This doesn't imply we are going to change it to int, right? Since all the histograms are floats (duration)
There was a problem hiding this comment.
No, I'm removing ints from this API since Istio is only using floats. Just noticing it...
monitoring/monitoring.go
Outdated
| // Otel uses attribute.Int("name", val) and similar - from the stable package - | ||
| // but doesn't create a new Metric, it is an option to Add(). |
There was a problem hiding this comment.
Not pre-creating objects with the attributes is what was costing Istiod to spend ~25% of its time in metrics code. So hopefully this is not suggesting we do that?
There was a problem hiding this comment.
This PR is not changing anything in the current API or OC implementation.
I have a separate change to use prometheus client - and it is also pre-computing the attribute.
This is a note that OTel metrics does things differently - fortunately it is not yet marked as stable and the
prom client is not deprecated, so not a concern - but it is important to keep this in mind. Attributes/labels
are the highest cost and complexity in all telemetry ( and logs ).
monitoring/monitoring.go
Outdated
| } | ||
|
|
||
| // NewGauge creates a new Metric with an aggregation type of LastValue. That means that data collected | ||
| // by the new Metric will export only the last recorded value. | ||
| func NewGauge(name, description string, opts ...Options) Metric { | ||
| return newMetric(name, description, view.LastValue(), opts...) | ||
| if strings.HasSuffix(name, "_total") { | ||
| fmt.Println(name) |
There was a problem hiding this comment.
Yes, I was trying to find bad names - we can't really change them.
| @@ -0,0 +1,229 @@ | |||
| //go:build !skip_opencensus | |||
There was a problem hiding this comment.
Whats the long term plan for this? its not easy to set build tags in istio release today. But not a big deal if its short lived
There was a problem hiding this comment.
Since OC is deprecated - it has to be short lived :-)
A separate PR will add an implementation based on prom client, and swap the default - I'm not sure if we should have an env variable to control or just switch, will get there - one step at a time, but I do not plan to have this in the release.
There was a problem hiding this comment.
Why prom? I thought we were moving to otel?
There was a problem hiding this comment.
Why not use otel SDK to allow exporters besides prometheus? Semantically they should be equivalent per their stated goal.
|
@costinm: The following tests failed, say
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. |
|
OTel metrics are not yet 'stable' - while prom is a dependency we already
have. Even with otel API, exporting to prometheus is still based on the
prometheus client library, by registering a 'generator'.
We can still use otel traces which are v1+ - but in practice registering a
counter/gauge/historgram with the prometheus client library will continue
to work even if we start
registering some counters with OTel API when it moves to v1 - and we have
an opportunity to validate the performance and impact.
We can also start using OTel collector - which supports reading prom
metrics and pushing to OTLP endpoints.
…On Mon, May 8, 2023 at 12:09 PM John Howard ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In monitoring/monitoring_opencensus.go
<#804 (comment)>:
> @@ -0,0 +1,229 @@
+//go:build !skip_opencensus
Why prom? I thought we were moving to otel?
—
Reply to this email directly, view it on GitHub
<#804 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAUR2XC7DMA6S7AFE3D3M3XFFAHPANCNFSM6AAAAAAX2HZ2AE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Few reasons:
1. Otel metric SDK is not yet 1.0/stable
2. "Stated goal" may or may not be equivalent with reality - we have a lot
of stated goals too
3. It is an interface - best to have multiple implementations, so we can
actually compare for ourselves.
4. Not clear if Istio should take the burden of configuring other exporters
(and their auth) instead of letting OTel-collector
or similar tools handle that - across k8s including non-istio workloads.
Same applies to istiod.
When Otel metric is stable/1.0 and we check the numbers - we may as well
start using the OTel API directly
instead of a wrapper ( I think we should do the same for log once new slog
is available ).
…On Tue, May 9, 2023 at 1:45 AM Kuat ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In monitoring/monitoring_opencensus.go
<#804 (comment)>:
> @@ -0,0 +1,229 @@
+//go:build !skip_opencensus
Why not use otel SDK to allow exporters besides prometheus? Semantically
they should be equivalent per their stated goal.
—
Reply to this email directly, view it on GitHub
<#804 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAUR2XM63ZOPL6SGD6H3Z3XFH737ANCNFSM6AAAAAAX2HZ2AE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
SGTM, as long as we maintain a wrapper and don't force any library until Otel is ready. |
|
Agreed, just like we want Istio users to only use stable features in
production and not experimental - we should do the same process for our
dependencies :-)
And even when OTel metrics is v1 - we would still want to test and compare
it with the prom library which is in broad use. Whatever is faster
and better can be used ( unless prometheus library gets deprecated ).
OTel tracing is v1 - and we can use it, and I believe for access logs there
is some gray area. There is also a subtle difference between
protocol and the client libraries - supporting OTLP and OTel is possible
without using a particular library.
…On Tue, May 9, 2023 at 9:09 AM Kuat ***@***.***> wrote:
SGTM, as long as we maintain a wrapper and don't force any library until
Otel is ready.
—
Reply to this email directly, view it on GitHub
<#804 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAUR2QLDZAI3K4TWN6EFFLXFJT43ANCNFSM6AAAAAAX2HZ2AE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
This is still WIP (commented out a number of unused methods, will cleanup).
Like the log change, there are a few small updates in istio repo, but very small. Almost all of Istio is using
the float variables and the methods removed are not used and will simplify migration from OC.