add otel-metrics operator for image-based gadgets #2900

flyth · 2024-05-27T10:48:59Z

This introduces a new otel-metrics operator for image-based gadgets. This operator can consume and export data from data sources / fields if they're correctly annotated:

DataSource needs "metrics.export" set to "true" to be considered by the operator
Fields need "metrics.type" set to either "key", "counter", "gauge" or "histogram"
- counter/gauge/histogram will export those fields using the specified type
- "key" can be used optionally (and multiple times) to group metrics by the unique set of keys given
optionally, "metrics.description" and "metrics.unit" can be used to provide some metadata to the exporter
fields without a "metrics.type" will be ignored

To actually enable metrics export, the otel-metrics-name param must be set to the exported name explicitly. This ensures a stable export name. Open Telemetry suggests:

This name uniquely identifies the [instrumentation scope](https://opentelemetry.io/docs/specs/otel/glossary/#instrumentation-scope), such as the [instrumentation library](https://opentelemetry.io/docs/specs/otel/glossary/#instrumentation-library) (e.g. io.opentelemetry.contrib.mongodb), package, module or class name. If an application or library has built-in OpenTelemetry instrumentation, both [Instrumented library](https://opentelemetry.io/docs/specs/otel/glossary/#instrumented-library) and [Instrumentation library](https://opentelemetry.io/docs/specs/otel/glossary/#instrumentation-library) can refer to the same library. In that scenario, the name denotes a module name or component name within that library or application.

I think its good to leave the decision up to the user, since it's effectively them creating the scope by using a specifically configured gadget.

If enabled, the operator will listen on an HTTP port and provide the prometheus-like interface.

A demo to use it from the Go API has been included.

With the recent update to the otel packages, the workarounds for synchronous gauges could finally be removed and the code further simplified.

Functionality that will automatically annotate/export eBPF maps will follow in a separate PR.

Todo

Proper documentation of annotations
add IG internal metrics [RFE] Exporting internal IG metrics to Prometheus #2981

eiffel-fl

Hi!

This is looking great as this is something which would help us with regard to prometheus.
I have nonetheless several comments and I also would like how does it articulate with #2425.
Particularly as this PR adds support for histogram, so I am wondering if I can make use of them to revive the profiler support.

Best regards.

.github/workflows/inspektor-gadget.yml

eiffel-fl · 2024-05-28T08:27:15Z

pkg/operators/otel-metrics/otel-metrics.go

+const (
+	name = "otel-metrics"
+
+	Priority                      = 50000


I am wondering if we should not have a global Priority we share between package and we just increase this value each time for new operator (except those which need to have a very low priority).
Because with all of this hardcoded values, we are taking the highway to future problems and this would not be easy to detect.

Absolutely agree.

pkg/operators/otel-metrics/otel-metrics.go

eiffel-fl · 2024-05-28T08:30:24Z

pkg/operators/otel-metrics/otel-metrics.go

+		go func() {
+			mux := http.NewServeMux()
+			mux.Handle("/metrics", promhttp.Handler())
+			err := http.ListenAndServe(globalParams.Get(ParamOtelMetricsListenAddress).AsString(), mux)


Would this use https?

No, it's using http, which is the default for serving prometheus metrics "internally". We should add an option to be able to use https later on, but that requires certificates to be present and we should plan a proper solution that then could also secure the gRPC endpoints - this is especially tricky on Kubernetes (cert-manager, etc.).

pkg/operators/otel-metrics/otel-metrics.go

examples/gadgets/operators/otel_metrics/README.md

eiffel-fl · 2024-05-28T08:46:24Z

examples/gadgets/operators/otel_metrics/README.md

+uses a counter, gauge and a histogram that emits new values every second. With the provided
+annotations, the operator will set those fields up properly and export them.
+
+Note that in this demo no actual gadget is run (thus the image parameter is unused) - instead,


I suppose people would use it with an image?
So, is there any possibilities to craft a toy image to display this?
If this is not that simple, let's stick with what we have right now.

Most likely - we just don't have any right now, so I kept this as simple as possible to get things started. I'd like to replace it later on once we have a proper example.

pkg/operators/otel-metrics/otel-metrics.go

flyth · 2024-05-28T18:15:21Z

Thanks for the feedback!

I have nonetheless several comments and I also would like how does it articulate with #2425.
Particularly as this PR adds support for histogram, so I am wondering if I can make use of them to revive the profiler support.

A follow-up PR will bring a generic solution that should also make it possible to implement profiler gadgets. Later on we could add support for printing histograms to the CLI (or another dedicated) operator (but I haven't started working on that one, yet).

eiffel-fl

Hi!

I only found nits but I would like to get a better understanding of it.
Also another review from someone else more familiar with open telemetry would be welcomed.

Best regards.

pkg/operators/otel-metrics/otel-metrics.go

eiffel-fl · 2024-05-30T10:06:26Z

pkg/operators/otel-metrics/otel-metrics.go

+			continue
+		}
+
+		metricsName := gadgetCtx.ImageName() // TODO: make this more unique?


What is the risk here? That two metrics share the same name?
If so, just happen some random number or have a static int.

Usually if you're exporting/collecting metrics, you want to have the name stable in order to have a continuous series of data. If you're running multiple gadgets of the same type with different configs under the same name (same as not configured in the current implementation), that will most likely skew the data. The uniqueness could be achieved by appending for example part of the hash of the configuration. Just counting up wouldn't be stable, especially in the kubernetes environment (each node could have different counters).

The uniqueness could be achieved by appending for example part of the hash of the configuration.

OK, but shouldn't we rather explode if the different configs have the same name?

It'd be hard to keep track of the names, especially again in a kubernetes environment. But we could make the name mandatory, so that it can't happen by accident (or go with the hash route).

I should probably also add a parameter so that one can override the name per gadget run and not have it tied only to the gadget.yaml.

It'd be hard to keep track of the names, especially again in a kubernetes environment. But we could make the name mandatory, so that it can't happen by accident (or go with the hash route).

We can go with adding the hash, but we should print a warning to inform users we change the name to make it unique, so no surprises on their side.

I should probably also add a parameter so that one can override the name per gadget run and not have it tied only to the gadget.yaml.

This seems a good idea!

I changed the behavior slightly. Now the annotation metrics.export (previously metrics.enable) tells the exporter to consider a datasource for exporting. Setting the name using the parameter is now mandatory. This helps preventing accidental duplicates IMHO.

mauriciovasquezbernal

It's looking very good. I'm looking forward to see how it can be used from the gadgets.

examples/gadgets/operators/otel_metrics/README.md

examples/gadgets/operators/otel_metrics/.gitignore

pkg/operators/otel-metrics/otel-metrics.go

pkg/operators/otel-metrics/otel-metrics_test.go

pkg/operators/otel-metrics/otel-metrics.go

pkg/operators/otel-metrics/otel-metrics_test.go

mauriciovasquezbernal · 2024-06-13T19:42:30Z

pkg/operators/otel-metrics/otel-metrics_test.go

+	ctx, cancel := context.WithTimeout(context.TODO(), time.Second)
+	defer cancel()
+
+	expectedBuckets := []uint64{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 3, 2, 3, 0}


Perhaps use AnnotationMetricsBoundaries to also test it?

pkg/operators/otel-metrics/otel-metrics.go

eiffel-fl

Hi!

I have some comments.

Best regards.

eiffel-fl · 2024-06-21T13:19:20Z

pkg/operators/otel-metrics/otel-metrics.go

+	switch f.Type() {
+	default:
+		// This should not be called with types other than int
+		panic("unsupported field type for asInt64")


You can have this function return an error instead.
Moreover, you would then be able to return the underlying error of calling extract() some lines above.

It's a code path that shouldn't be used (validation should happen (and happens) in the callers of this function) - that's why I opted not to have runtime error handling here. Should this ever be used, it will let us know immediately.

It's a code path that shouldn't be used

If this is not used, then simply remove it.

I think it's better to have these "asserts" in (that would crash and instantly notify us), rather than just returning with an unexpected 0 in those cases (should anyone ever use it in a wrong way.)

eiffel-fl · 2024-06-21T13:19:32Z

pkg/operators/otel-metrics/otel-metrics.go

+	switch f.Type() {
+	default:
+		// This should not be called with types other than float
+		panic("unsupported field type for asFloat4")


Same comment.

pkg/operators/otel-metrics/otel-metrics.go

examples/gadgets/operators/otel_metrics/main.go

This adds a new operator that - based on data source and field annotations - can export fields to an Open Telemetry metrics interface (Prometheus). Signed-off-by: Michael Friese <mfriese@microsoft.com>

Signed-off-by: Michael Friese <mfriese@microsoft.com>

flyth force-pushed the michael/oci/otel-metrics branch 2 times, most recently from e2aaf57 to 51c13e0 Compare May 27, 2024 17:29

flyth marked this pull request as ready for review May 28, 2024 07:48

flyth requested review from mauriciovasquezbernal, eiffel-fl and mqasimsarfraz as code owners May 28, 2024 07:48

eiffel-fl reviewed May 28, 2024

View reviewed changes

pkg/operators/otel-metrics/otel-metrics.go Outdated Show resolved Hide resolved

flyth force-pushed the michael/oci/otel-metrics branch from 51c13e0 to 5a1da64 Compare May 28, 2024 18:10

flyth force-pushed the michael/oci/otel-metrics branch from 5a1da64 to 35bfa23 Compare May 30, 2024 06:46

eiffel-fl reviewed May 30, 2024

View reviewed changes

flyth force-pushed the michael/oci/otel-metrics branch 3 times, most recently from 382f825 to 0b574d7 Compare May 30, 2024 17:03

mauriciovasquezbernal reviewed Jun 13, 2024

View reviewed changes

flyth force-pushed the michael/oci/otel-metrics branch 3 times, most recently from db181ef to 74cf555 Compare June 21, 2024 09:38

eiffel-fl reviewed Jun 21, 2024

View reviewed changes

pkg/operators/otel-metrics/otel-metrics.go Show resolved Hide resolved

pkg/operators/otel-metrics/otel-metrics.go Show resolved Hide resolved

eiffel-fl reviewed Jun 21, 2024

View reviewed changes

flyth force-pushed the michael/oci/otel-metrics branch from 74cf555 to 76340db Compare June 21, 2024 15:50

flyth added 2 commits June 26, 2024 15:21

add otel-metrics exporter operator

aadc265

This adds a new operator that - based on data source and field annotations - can export fields to an Open Telemetry metrics interface (Prometheus). Signed-off-by: Michael Friese <mfriese@microsoft.com>

add otel-metrics exporter example

4960c8b

Signed-off-by: Michael Friese <mfriese@microsoft.com>

flyth force-pushed the michael/oci/otel-metrics branch from 76340db to 4960c8b Compare June 26, 2024 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add otel-metrics operator for image-based gadgets #2900

add otel-metrics operator for image-based gadgets #2900

flyth commented May 27, 2024 •

edited

Loading

eiffel-fl left a comment

eiffel-fl May 28, 2024

flyth May 28, 2024

eiffel-fl May 28, 2024

flyth May 28, 2024

eiffel-fl May 28, 2024

flyth May 28, 2024

flyth commented May 28, 2024

eiffel-fl left a comment

eiffel-fl May 30, 2024

flyth May 30, 2024

eiffel-fl May 30, 2024

flyth May 30, 2024

eiffel-fl May 30, 2024

flyth May 30, 2024

mauriciovasquezbernal left a comment

mauriciovasquezbernal Jun 13, 2024

eiffel-fl left a comment

eiffel-fl Jun 21, 2024

flyth Jun 21, 2024

eiffel-fl Jun 24, 2024

flyth Jun 26, 2024

eiffel-fl Jun 21, 2024

add otel-metrics operator for image-based gadgets #2900

Are you sure you want to change the base?

add otel-metrics operator for image-based gadgets #2900

Conversation

flyth commented May 27, 2024 • edited Loading

Todo

eiffel-fl 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyth commented May 28, 2024

eiffel-fl 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eiffel-fl 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

Choose a reason for hiding this comment

flyth commented May 27, 2024 •

edited

Loading