stat_sinks: add OpenTelemetry stats sink

[ohadvano]

Commit Message: stat_sinks: OpenTelemetry stats sink
Additional Description: Adding OpenTelemetry stats sink, following the OTLP protocol and proto definitions.
Risk Level: Low
Testing: Unit tests, Integration tests
Docs Changes: Changelog, Stats sinks docs
Release Notes: None
Platform Specific Features: None

Resolves #26498

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #26620 was opened by ohadvano.

see: more, trace.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @RyanTheOptimist

🐱

Caused by: #26620 was opened by ohadvano.

see: more, trace.

[ohadvano]

I am opening this PR for initial review, before proceeding to tests implementation. Will appreciate your help with initial review.
/assign-from @envoyproxy/senior-maintainers

@mattklein123 I have temporarily mentioned you in the CODEOWNERS for that new extension, just to unblock the CI (or not temporarily if requested of course).

[repokitteh-read-only]

@envoyproxy/senior-maintainers assignee is @mattklein123

🐱

Caused by: a #26620 (comment) was created by @ohadvano.

see: more, trace.

[mattklein123]

Seems fine at a high level. Can you add tests, etc. and we can go from there?

/wait

[RyanTheOptimist]

/lgtm deps

[kyessenov]

Very cool. Did you think of the semantic conversion support (https://opentelemetry.io/docs/reference/specification/compatibility/prometheus_and_openmetrics/)? For example, stripping _bucket and _count suffixes.

[ohadvano]

Very cool. Did you think of the semantic conversion support (https://opentelemetry.io/docs/reference/specification/compatibility/prometheus_and_openmetrics/)? For example, stripping _bucket and _count suffixes.

Thanks for referring me to this doc, I haven't got to it yet. I'm curious however what is the motivation for supporting this? As I see it, Envoy is not implementing Prometheus semantics in first place? Also if someone chooses this sink then I would assume they would want the semantics to be with OTLP in first place, so what does Prometheus has to do with it? I guess we could add Prometheus sink dedicated for that protocol..

[kyessenov]

@ohadvano You are right that Envoy stats pre-date conventions of prometheus. But looking forward, metrics are likely to be added that follow either prometheus or Otel conventions. For example, Istio added istio_requests_total in its custom extension, which is in "prometheus" style, so it'd fail "otel" style when converted verbatim. But if you write "otel" style metrics directly, there's no problem with "otel" sink. Perhaps, we can go the other way and instead assume all new metrics are "otel" style and convert to "prometheus" style in the "prometheus" sink. BTW I don't think this need to be solved now, I was curious to hear your thoughts.

[ohadvano]

For example, Istio added istio_requests_total in its custom extension, which is in "prometheus" style, so it'd fail "otel" style when converted verbatim

What would be the expected behavior in such case if OTEL sink is being used, or alternatively asking what do you mean by 'fail'?

[kyessenov]

Per Otel semantic conventions: If the counter has a _total suffix, it MUST be removed.
The other one I know is: duration must be in seconds, and _milliseconds suffix must be stripped and unit converted.

[ohadvano]

Per Otel semantic conventions: If the counter has a _total suffix, it MUST be removed. The other one I know is: duration must be in seconds, and _milliseconds suffix must be stripped and unit converted.

Yeah but I mean, what should happen in case you "violate" this convention and decide to name your counter with _total suffix anyways? I'm asking this because currently Envoy has some counter stats with this name, for example: downstream_cx_total. Is it expected that the collector will drop these metrics? I couldn't find any other doc that describes this, also couldn't find any other doc that specifies these conventions regardless of Prometheus, so will appreciate if you could refer me to any.

[ohadvano]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #26620 (comment) was created by @ohadvano.

see: more, trace.

[mattklein123]

Some high level comments to start out with, thanks.

/wait

[ohadvano]

/coverage

[repokitteh-read-only]

Coverage for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/26620/coverage/index.html

The coverage results are (re-)rendered each time the CI envoy-presubmit (check linux_x64 coverage) job completes.

🐱

Caused by: a #26620 (comment) was created by @ohadvano.

see: more, trace.

[mattklein123]

LGTM at a high level. Some small comments, thanks.

/wait

[mattklein123]

Thanks!

[ohadvano]

@mattklein123 I think this won't merge because of the API approval check, can you please approve it as well?

[ohadvano]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #26620 (comment) was created by @ohadvano.

see: more, trace.