[Stack Monitoring] Add OpenTelemetry metrics to Monitoring Collection plugin #135999

crespocarlos · 2022-07-08T11:47:11Z

Summary

This PR creates Otel integration in Kibana and closes https://github.com/elastic/observability-dev/issues/2220. It's based on this PoC #133171, but removes the instrumentation code part, leaving only the essential code to enable the integration.

Screenshots

With Prometheus endpoint enabled

With the OpenTelemetry Metrics API exported as OpenTelemetry protocol enabled

Review notes

Since we're going with versioned API for Prometheus endpoint, I refactored the monitoring_collection plugin routes folder to organise it better, following the standard found in monitoring plugin
Unfortunately just by following the steps described in README.md, there won't be any metrics generated by Otel integration. In order to have those, we need to instrument the code - for testing purposes. Basically, copy the content from alerting/monitoring folder from the PoC PR https://github.com/elastic/kibana/pull/133171/files

Todo

While testing this, I found a possible bug in the Prometheus package that causes histogram type fail to be ingested

elasticmachine · 2022-07-08T13:21:45Z

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

matschaffer · 2022-07-11T03:27:11Z

@crespocarlos I'm not sure if we're gaining much by keeping my PoC commit history here. Did you include it intentionally?

matschaffer

Did you explore adding an API integration test? I'm not sure, but if there's a way to inject a dummy plugin during the integration testing cycle, we could use that as a test-bed for instrumentation techniques.

matschaffer · 2022-07-11T03:27:41Z

package.json

@@ -263,6 +264,12 @@
    "@mapbox/mapbox-gl-draw": "1.3.0",
    "@mapbox/mapbox-gl-rtl-text": "0.2.3",
    "@mapbox/vector-tile": "1.3.1",
+    "@opentelemetry/api-metrics": "0.29.2",
+    "@opentelemetry/exporter-metrics-otlp-grpc": "^0.29.2",


We should be able to set these all to 0.30.0 I think.

matschaffer · 2022-07-11T03:29:05Z

x-pack/plugins/monitoring_collection/README.md

+## OpenTelemetry Metrics
+
+TODO: explain how to instrument the code with `@opentelemetry/api-metrics` so that the steps below will work with metrics


We'll need to do this to review the PR, so might be good to include something simple like a plugin setup counter.

yeah. I've added a very simple example to it. let me know if it's clear enough

crespocarlos · 2022-07-11T07:49:02Z

@crespocarlos I'm not sure if we're gaining much by keeping my PoC commit history here. Did you include it intentionally?

I didn't pay attention to that. I can squash those

Did you explore adding an API integration test? I'm not sure, but if there's a way to inject a dummy plugin during the integration testing cycle, we could use that as a test-bed for instrumentation techniques.

I did. ~~At first it seemed too complex to get all things in place, especially alerting to see any sort of data~~ (I haven't thought about a dummy plugin). But I can definitely send more time here.

…tlp-grpc versions to 0.30.0

x-pack/test/api_integration/apis/monitoring_collection/prometheus.ts

test/common/fixtures/plugins/otel_metrics/kibana.json

crespocarlos · 2022-07-11T20:43:22Z

@elasticmachine merge upstream

matschaffer · 2022-07-12T03:04:18Z

x-pack/test/api_integration/config.ts

+        `--monitoring_collection.opentelemetry.metrics=${JSON.stringify({
+          prometheus: {
+            enabled: true,
+          },
+        })}`,


Suggested change

`--monitoring_collection.opentelemetry.metrics=${JSON.stringify({

prometheus: {

enabled: true,

},

})}`,

`--monitoring_collection.opentelemetry.metrics.prometheus.enabled=true`,

Pretty sure this should work too and maybe simpler.

Yep. Confirmed locally.

x-pack/test/api_integration/apis/monitoring_collection/index.ts

matschaffer · 2022-07-12T06:57:46Z

x-pack/plugins/monitoring_collection/server/lib/prometheus_exporter.ts

+export class PrometheusExporter extends MetricReader {
+  private readonly _prefix?: string;
+  private readonly _appendTimestamp: boolean;
+  private _serializer: PrometheusSerializer;


I think this can be just _serializer - pretty sure the underscore is just copy-pasta from the otel js repo during my PoC work.

you mean without the type? It can, but then TS will understand that this an any. Better to define the type as we're currently doing

without the underscore I mean :)

For some reason my brain didn't read the underscore from your original comment lol. Yeah, I'll remove the underscore from these properties.

It doesn't hurt to leave them, but there are more examples without _ than with in the codebase.

matschaffer

Looking good overall, just some recommendations for polishing.

matschaffer · 2022-07-12T07:00:13Z

x-pack/plugins/monitoring_collection/server/routes/api/v1/dynamic_route/index.test.ts

@@ -5,7 +5,7 @@
 * 2.0.
 */

-import { registerDynamicRoute } from './dynamic_route';
+import { registerDynamicRoute } from '.';


Looks like the idea here is to get both the prometheus and dynamic route under a v1 directory. I'd probably leave that alone to keep the scope of the PR just on the otel stuff.

I wanted to leave it alone too, but it looked too ugly, that's why I decided to move that to v1 folder.

I based it on how monitoring plugin organizes its api folder. Leaving both in the root folder felt weird, and moving only prometheus to v1 seemed weird too.

matschaffer · 2022-07-12T07:05:55Z

package.json

+    "@opentelemetry/exporter-metrics-otlp-grpc": "^0.30.0",
+    "@opentelemetry/exporter-prometheus": "^0.30.0",
+    "@opentelemetry/resources": "^1.3.1",
+    "@opentelemetry/sdk-metrics-base": "^0.29.2",


let's get all the 0.29s out of here and just focus on 0.30

matschaffer · 2022-07-12T07:06:32Z

x-pack/test/api_integration/config.ts

+        `--monitoring_collection.opentelemetry.metrics=${JSON.stringify({
+          prometheus: {
+            enabled: true,
+          },
+        })}`,


Yep. Confirmed locally.

matschaffer · 2022-07-13T05:17:59Z

x-pack/plugins/monitoring_collection/server/plugin.ts

+
+      const credentials = url.startsWith('https://')
+        ? grpc.credentials.createSsl()
+        : grpc.credentials.createInsecure();


Pretty sure with 0.30 we can drop this (and the grpc direct dependency)

I opened crespocarlos#1 with the proposed change - still need to test it with/without SSL

Sadly looks like the grpc dependency is still required to set headers, but at least we don't have to do our own secure/insecure check anymore.

x-pack/plugins/monitoring_collection/README.md

x-pack/plugins/monitoring_collection/server/routes/api/v1/dynamic_route/index.test.ts

matschaffer · 2022-07-13T05:58:34Z

@elasticmachine merge upstream

x-pack/plugins/monitoring_collection/server/routes/api/v1/prometheus/index.ts

matschaffer · 2022-07-13T06:10:29Z

x-pack/plugins/monitoring_collection/server/plugin.ts

+
+      const credentials = url.startsWith('https://')
+        ? grpc.credentials.createSsl()
+        : grpc.credentials.createInsecure();


I opened crespocarlos#1 with the proposed change - still need to test it with/without SSL

Review: [Stack Monitoring] Add OpenTelemetry metrics to Monitoring Collection plugin

matschaffer

Let's open a follow up issue for the https://opentelemetry.io/docs/reference/specification/protocol/exporter/ env vars. This is plenty good to merge (once you've sorted the merge conflict of course).

Great work @crespocarlos !

matschaffer · 2022-07-14T06:00:17Z

x-pack/plugins/monitoring_collection/server/plugin.ts

+    metrics.setGlobalMeterProvider(meterProvider);
+
+    const otlpConfig = this.config.opentelemetry?.metrics.otlp;
+    const url = otlpConfig?.url ?? process.env.OTEL_EXPORTER_OTLP_METRICS_ENDPOINT;


OTEL_EXPORTER_OTLP_ENDPOINT should be allowable too. It'll be convenient once we align traces and metrics both on the otel spec. Could open that as a follow up issue.

I will add the OTEL_EXPORTER_OTLP_ENDPOINT - I forgot about it. It's a small change.

…cs-alerting-plugin

kibana-ci · 2022-07-14T09:18:01Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #57409 succeeded 9ab2f05
💚 Build #57268 succeeded 533bf45
💚 Build #57259 succeeded 0c71fa9
💚 Build #56932 succeeded f4340ed
💚 Build #56844 succeeded b595d89
💔 Build #56836 failed 70d0ed0

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

crespocarlos changed the title ~~2220 otel metrics alerting plugin~~ [Stack Monitoring] Add OpenTelemetry metrics to Monitoring Collection plugin Jul 8, 2022

crespocarlos marked this pull request as ready for review July 8, 2022 13:21

crespocarlos requested a review from a team July 8, 2022 13:21

matschaffer reviewed Jul 11, 2022

View reviewed changes

matschaffer and others added 2 commits July 11, 2022 10:36

Add otel metrics to alerting plugin

d911a31

clean up otel poc

2638cbb

crespocarlos force-pushed the 2220-otel-metrics-alerting-plugin branch from 6ffd8f6 to 2638cbb Compare July 11, 2022 08:36

crespocarlos added 2 commits July 11, 2022 10:38

Bump @opentelemetry/api-metrics and @opentelemetry/exporter-metrics-o…

218a4ff

…tlp-grpc versions to 0.30.0

Add integration test for prometheus endpoint; improve reademe.md

70d0ed0

crespocarlos commented Jul 11, 2022

View reviewed changes

x-pack/test/api_integration/apis/monitoring_collection/prometheus.ts Show resolved Hide resolved

crespocarlos commented Jul 11, 2022

View reviewed changes

test/common/fixtures/plugins/otel_metrics/kibana.json Show resolved Hide resolved

Fix tsconfig.base.json missing entries

bb77bc1

Merge branch 'main' into 2220-otel-metrics-alerting-plugin

b595d89

matschaffer reviewed Jul 12, 2022

View reviewed changes

matschaffer mentioned this pull request Jul 12, 2022

[Proof of Concept] Add otel metrics to alerting plugin #133171

Closed

10 tasks

matschaffer reviewed Jul 12, 2022

View reviewed changes

x-pack/test/api_integration/apis/monitoring_collection/index.ts Show resolved Hide resolved

matschaffer reviewed Jul 12, 2022

View reviewed changes

matschaffer suggested changes Jul 12, 2022

View reviewed changes

crespocarlos added 2 commits July 12, 2022 10:30

Bump @opentelemetry/sdk-metrics-base; clean up

4ed979b

Rename PrometheusExporter properties

f4340ed

crespocarlos requested a review from matschaffer July 12, 2022 08:42

matschaffer reviewed Jul 13, 2022

View reviewed changes

x-pack/plugins/monitoring_collection/README.md Show resolved Hide resolved

Readme formatting tweaks

67e5a64

matschaffer reviewed Jul 13, 2022

View reviewed changes

x-pack/plugins/monitoring_collection/server/routes/api/v1/dynamic_route/index.test.ts Outdated Show resolved Hide resolved

Fix incorrect path

6812f29

kibanamachine and others added 2 commits July 13, 2022 01:58

Merge branch 'main' into 2220-otel-metrics-alerting-plugin

0c71fa9

Remove grpc dependency

b716f3a

matschaffer mentioned this pull request Jul 13, 2022

Review: [Stack Monitoring] Add OpenTelemetry metrics to Monitoring Collection plugin crespocarlos/kibana#1

Merged

matschaffer reviewed Jul 13, 2022

View reviewed changes

matschaffer and others added 6 commits July 13, 2022 15:56

Add grpc back for handling auth headers

92a0862

Fix comment positioning

0991876

Include authenticated OTLP in readme

76b7780

Merge pull request #1 from matschaffer/2220-otel-metrics-alerting-plugin

052b9dd

Review: [Stack Monitoring] Add OpenTelemetry metrics to Monitoring Collection plugin

Extract dynamic route into a new file

533bf45

Enable otlp logging and compatibility with env vars

9ab2f05

matschaffer approved these changes Jul 14, 2022

View reviewed changes

crespocarlos added 2 commits July 14, 2022 10:10

Merge branch 'main' of github.com:elastic/kibana into 2220-otel-metri…

5c409c4

…cs-alerting-plugin

Enable OTEL_EXPORTER_OTLP_ENDPOINT env var

34f19d5

crespocarlos merged commit b58d07e into elastic:main Jul 14, 2022

crespocarlos deleted the 2220-otel-metrics-alerting-plugin branch July 14, 2022 11:29

jennypavlova mentioned this pull request Nov 7, 2024

Categorise Kibana plugins - Wave 2 #199302

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Stack Monitoring] Add OpenTelemetry metrics to Monitoring Collection plugin #135999

[Stack Monitoring] Add OpenTelemetry metrics to Monitoring Collection plugin #135999

crespocarlos commented Jul 8, 2022

elasticmachine commented Jul 8, 2022

matschaffer commented Jul 11, 2022

matschaffer left a comment

matschaffer Jul 11, 2022

crespocarlos Jul 11, 2022

matschaffer Jul 11, 2022

crespocarlos Jul 11, 2022

crespocarlos commented Jul 11, 2022 •

edited

Loading

crespocarlos commented Jul 11, 2022

matschaffer Jul 12, 2022

matschaffer Jul 12, 2022

matschaffer Jul 12, 2022

crespocarlos Jul 12, 2022 •

edited

Loading

matschaffer Jul 12, 2022

crespocarlos Jul 12, 2022

crespocarlos Jul 12, 2022

matschaffer left a comment

matschaffer Jul 12, 2022

crespocarlos Jul 12, 2022 •

edited

Loading

crespocarlos Jul 12, 2022

matschaffer Jul 12, 2022

matschaffer Jul 12, 2022

matschaffer Jul 13, 2022

matschaffer Jul 13, 2022

matschaffer Jul 13, 2022

matschaffer commented Jul 13, 2022

matschaffer Jul 13, 2022

matschaffer left a comment

matschaffer Jul 14, 2022

crespocarlos Jul 14, 2022

kibana-ci commented Jul 14, 2022

		## OpenTelemetry Metrics

		TODO: explain how to instrument the code with `@opentelemetry/api-metrics` so that the steps below will work with metrics

[Stack Monitoring] Add OpenTelemetry metrics to Monitoring Collection plugin #135999

[Stack Monitoring] Add OpenTelemetry metrics to Monitoring Collection plugin #135999

Conversation

crespocarlos commented Jul 8, 2022

Summary

Screenshots

With Prometheus endpoint enabled

With the OpenTelemetry Metrics API exported as OpenTelemetry protocol enabled

Review notes

Todo

elasticmachine commented Jul 8, 2022

matschaffer commented Jul 11, 2022

matschaffer 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

crespocarlos commented Jul 11, 2022 • edited Loading

crespocarlos commented Jul 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crespocarlos Jul 12, 2022 • edited Loading

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

matschaffer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crespocarlos Jul 12, 2022 • edited Loading

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

matschaffer commented Jul 13, 2022

Choose a reason for hiding this comment

matschaffer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Jul 14, 2022

💚 Build Succeeded

Metrics [docs]

History

crespocarlos commented Jul 11, 2022 •

edited

Loading

crespocarlos Jul 12, 2022 •

edited

Loading

crespocarlos Jul 12, 2022 •

edited

Loading