Open Telemetry integration #1284

marcopiraccini · 2023-07-20T12:56:24Z

This for the open telemetry integration
It's a WIP, missing:

mcollina

Unfortunately this uses the async_hooks based context manager, which slows things down quite drastically (15-20%), which is really not something we should be encouraging.

We should use the "manual" module that those use and just plug into Fastify hooks ourselves.

mcollina · 2023-07-20T12:59:34Z

packages/telemetry/index.js

@@ -0,0 +1,79 @@
+const fp = require('fastify-plugin')


Suggested change

const fp = require('fastify-plugin')

'use strict'

const fp = require('fastify-plugin')

mcollina · 2023-07-20T13:03:20Z

packages/telemetry/index.js

@@ -0,0 +1,79 @@
+const fp = require('fastify-plugin')
+const { registerInstrumentations } = require('@opentelemetry/instrumentation')


don't use this module

mcollina · 2023-07-20T13:03:28Z

packages/telemetry/index.js

@@ -0,0 +1,79 @@
+const fp = require('fastify-plugin')
+const { registerInstrumentations } = require('@opentelemetry/instrumentation')
+const { HttpInstrumentation } = require('@opentelemetry/instrumentation-http')


don't use this module

mcollina · 2023-07-20T13:03:36Z

packages/telemetry/index.js

+const { registerInstrumentations } = require('@opentelemetry/instrumentation')
+const { HttpInstrumentation } = require('@opentelemetry/instrumentation-http')
+const { ConsoleSpanExporter, BatchSpanProcessor, SimpleSpanProcessor, InMemorySpanExporter } = require('@opentelemetry/sdk-trace-base')
+const { NodeTracerProvider } = require('@opentelemetry/sdk-trace-node')


don't use this module

mcollina · 2023-07-22T09:19:38Z

packages/telemetry/index.js

+    return {
+      get activeSpan () {
+        return request.span
+      }


why is this a getter instead of a property?

mcollina · 2023-07-22T09:20:27Z

packages/telemetry/index.js

+  }
+
+  // expose the API as a request decorator
+  app.decorateRequest('openTelemetry', openTelemetry)


Suggested change

app.decorateRequest('openTelemetry', openTelemetry)

app.decorateRequest('openTelemetry', openTelemetry)

app.decorateRequest('span') // Tells V8 to optimize this property

mcollina · 2023-07-22T09:23:05Z

packages/telemetry/index.js

+  const spanProcessor = ['memory', 'console'].includes(exporter.type) ? new SimpleSpanProcessor(exporterObj) : new BatchSpanProcessor(exporterObj)
+  provider.addSpanProcessor(spanProcessor)
+  const tracer = provider.getTracer(moduleName, moduleVersion)
+  return { tracer, exporter: exporterObj }


Suggested change

return { tracer, exporter: exporterObj }

return { tracer, exporter: exporterObj, provider }

mcollina · 2023-07-22T09:23:15Z

packages/telemetry/index.js

+}
+
+async function setupTelemetry (app, opts) {
+  const { tracer, exporter } = setupProvider(app, opts)


Suggested change

const { tracer, exporter } = setupProvider(app, opts)

const { tracer, exporter, provider } = setupProvider(app, opts)

mcollina · 2023-07-22T09:23:35Z

packages/telemetry/index.js

+async function setupTelemetry (app, opts) {
+  const { tracer, exporter } = setupProvider(app, opts)
+  // We decorate this to inspect it in tests
+  app.decorate('openTelemetryExporter', exporter)


I would include the full object returned by setupProvider here

mcollina · 2023-07-22T09:24:48Z

packages/telemetry/index.js

+  const { tracer, exporter } = setupProvider(app, opts)
+  // We decorate this to inspect it in tests
+  app.decorate('openTelemetryExporter', exporter)
+


Suggested change

app.addHook('onClose', async function () {

await provider.shutdown()

})

packages/telemetry/package.json

mcollina · 2023-07-22T09:25:38Z

packages/telemetry/schema.js

+  additionalProperties: false
+}
+
+module.exports = AuthSchema


Suggested change

module.exports = AuthSchema

module.exports = TelemetrySchema

packages/telemetry/test/multispan-processor.test.js

mcollina · 2023-07-22T09:26:01Z

packages/telemetry/test/platformatic-trace-provider.test.js

@@ -0,0 +1,45 @@
+const { test } = require('tap')


Suggested change

const { test } = require('tap')

'use strict'

const { test } = require('tap')

packages/telemetry/test/telemetry.test.js

mcollina · 2023-07-22T09:28:25Z

packages/telemetry/index.js

+  // We use a SimpleSpanProcessor for the console/memory exporters and a BatchSpanProcessor for the others.
+  const spanProcessor = ['memory', 'console'].includes(exporter.type) ? new SimpleSpanProcessor(exporterObj) : new BatchSpanProcessor(exporterObj)
+  provider.addSpanProcessor(spanProcessor)
+  const tracer = provider.getTracer(moduleName, moduleVersion)


I'm not sure we want to use moduleName moduleVersion here

This is by spec: https://opentelemetry.io/docs/specs/otel/trace/api/#get-a-tracer

mcollina

good work!

mcollina · 2023-07-25T08:35:24Z

packages/telemetry/index.js

@@ -0,0 +1,7 @@
+const telemetry = require('./telemetry')


Suggested change

const telemetry = require('./telemetry')

'use strict'

const telemetry = require('./telemetry')

mcollina · 2023-07-25T08:36:52Z

packages/telemetry/package.json

+    "@opentelemetry/resources": "^1.15.0",
+    "@opentelemetry/sdk-trace-base": "^1.15.0",
+    "@opentelemetry/semantic-conventions": "^1.15.0",
+    "fastify": "^4.20.0",


I don't think this is needed

mcollina · 2023-07-25T08:39:52Z

Regarding the service and db integration, I think we should be overriding each resolver and adding a wrapper that starts and close a span while it's processing it.

code is now ok!

mcollina · 2023-07-25T20:19:43Z

packages/client/package.json

@@ -28,6 +28,7 @@
    "typescript": "^5.1.3"
  },
  "dependencies": {
+    "@platformatic/telemetry": "workspace:*",


should be a devDep

mcollina · 2023-07-25T20:20:21Z

packages/client/test/telemetry.test.js

+  equal(clientSpan.attributes['response.statusCode'], 200)
+
+  // console.log('receivedHeaders', receivedHeaders)
+  // TODO: Check headers


mcollina · 2023-07-26T07:30:59Z

packages/client/index.js

-      query,
-      variables
+async function graphql (url, log, headers, query, variables, openTelemetry) {
+  const { span, headers: telemetryHeaders } = openTelemetry ? openTelemetry.startSpanClient(url.toString(), { method: 'POST' }) : { span: null, headers: {} }


I wonder if this propagates the headers correctly because the context is empty.
I think you need to get the OpenTelemetry context in

platformatic/packages/client/index.js

Lines 229 to 235 in 234330b

app.addHook('onRequest', async (req, reply) => {

const newClient = Object.create(client)

if (getHeaders) {

newClient[kGetHeaders] = getHeaders.bind(newClient, req, reply)

}

req[name] = newClient

})

, and then pass it down.

You are right, we need in client to actually recreate the context from the headers, otherwise in the case of:
C -> A -> B
...we don't propagte the traceId through A (a new one is created).
Fixing.

Fixed (and added a test: https://github.com/platformatic/platformatic/blob/opentel-integration/packages/client/test/telemetry.test.js#L78 )

packages/client/index.js

mcollina · 2023-07-28T10:36:20Z

code LGTM but there is a conflict and docs are missing.

Signed-off-by: marcopiraccini <marco.piraccini@gmail.com>

mcollina

Can you add a short guide on how to set up OpenTelemetry with Composer, Service and a Zipkin destination?

Signed-off-by: marcopiraccini <marco.piraccini@gmail.com>

mcollina

lgtm

Signed-off-by: marcopiraccini <marco.piraccini@gmail.com>

mcollina previously requested changes Jul 20, 2023

View reviewed changes

mcollina reviewed Jul 22, 2023

View reviewed changes

marcopiraccini force-pushed the opentel-integration branch 4 times, most recently from b621fd0 to 6a3dd58 Compare July 24, 2023 16:04

mcollina reviewed Jul 25, 2023

View reviewed changes

mcollina reviewed Jul 26, 2023

View reviewed changes

packages/client/index.js Outdated Show resolved Hide resolved

marcopiraccini changed the title ~~open telemetry integration [WIP]~~ Open Telemetry integration [WIP] Jul 27, 2023

marcopiraccini marked this pull request as ready for review July 27, 2023 08:49

marcopiraccini force-pushed the opentel-integration branch from 51de1ba to b356a22 Compare July 27, 2023 08:54

marcopiraccini changed the title ~~Open Telemetry integration [WIP]~~ Open Telemetry integration Jul 27, 2023

marcopiraccini force-pushed the opentel-integration branch from 647f350 to ae6560c Compare July 27, 2023 14:35

marcopiraccini force-pushed the opentel-integration branch from 1f125bb to 993cc01 Compare July 28, 2023 11:36

Open Telemetry integration

4c06da4

Signed-off-by: marcopiraccini <marco.piraccini@gmail.com>

marcopiraccini force-pushed the opentel-integration branch from 993cc01 to 1851842 Compare July 28, 2023 11:40

fixup for the c8 issue

ebdbf00

Signed-off-by: marcopiraccini <marco.piraccini@gmail.com>

marcopiraccini force-pushed the opentel-integration branch from 778799a to ebdbf00 Compare July 29, 2023 04:56

marcopiraccini added 2 commits July 29, 2023 07:10

removed useless commented test

e9df271

Signed-off-by: marcopiraccini <marco.piraccini@gmail.com>

reference documentation and mandatory service name

2c283fa

Signed-off-by: marcopiraccini <marco.piraccini@gmail.com>

mcollina reviewed Jul 31, 2023

View reviewed changes

marcopiraccini mentioned this pull request Jul 31, 2023

Telemetry guide and fix to disable orama in dev platformatic/docs#77

Merged

telemetry with jaeger guide

cc9b723

Signed-off-by: marcopiraccini <marco.piraccini@gmail.com>

marcopiraccini force-pushed the opentel-integration branch from a01b955 to cc9b723 Compare July 31, 2023 14:51

mcollina approved these changes Jul 31, 2023

View reviewed changes

marcopiraccini force-pushed the opentel-integration branch from 589706e to becf90a Compare August 1, 2023 03:41

fixups for c8 and node20

84825a7

Signed-off-by: marcopiraccini <marco.piraccini@gmail.com>

marcopiraccini force-pushed the opentel-integration branch from 483a341 to 84825a7 Compare August 1, 2023 04:46

mcollina merged commit 350774d into main Aug 1, 2023
78 checks passed

mcollina deleted the opentel-integration branch August 1, 2023 07:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Open Telemetry integration #1284

Open Telemetry integration #1284

marcopiraccini commented Jul 20, 2023 •

edited

mcollina left a comment

mcollina Jul 20, 2023

mcollina Jul 20, 2023

mcollina Jul 20, 2023

mcollina Jul 20, 2023

mcollina Jul 22, 2023

mcollina Jul 22, 2023

mcollina Jul 22, 2023

mcollina Jul 22, 2023

mcollina Jul 22, 2023

mcollina Jul 22, 2023

mcollina Jul 22, 2023

mcollina Jul 22, 2023

mcollina Jul 22, 2023

marcopiraccini Jul 24, 2023

mcollina left a comment

mcollina Jul 25, 2023

mcollina Jul 25, 2023

mcollina commented Jul 25, 2023

mcollina Jul 25, 2023

mcollina Jul 25, 2023

mcollina Jul 26, 2023

marcopiraccini Jul 26, 2023 •

edited

marcopiraccini Jul 26, 2023 •

edited

mcollina commented Jul 28, 2023

mcollina left a comment

mcollina left a comment

		@@ -0,0 +1,79 @@
		const fp = require('fastify-plugin')
		const { registerInstrumentations } = require('@opentelemetry/instrumentation')

	app.decorateRequest('openTelemetry', openTelemetry)
	app.decorateRequest('openTelemetry', openTelemetry)
	app.decorateRequest('span') // Tells V8 to optimize this property

	return { tracer, exporter: exporterObj }
	return { tracer, exporter: exporterObj, provider }

	const { tracer, exporter } = setupProvider(app, opts)
	const { tracer, exporter, provider } = setupProvider(app, opts)

+  app.addHook('onClose', async function () {
+    await provider.shutdown()
+  })

	app.addHook('onRequest', async (req, reply) => {
	const newClient = Object.create(client)
	if (getHeaders) {
	newClient[kGetHeaders] = getHeaders.bind(newClient, req, reply)
	}
	req[name] = newClient
	})

Open Telemetry integration #1284

Open Telemetry integration #1284

Conversation

marcopiraccini commented Jul 20, 2023 • edited

mcollina 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

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

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Jul 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcopiraccini Jul 26, 2023 • edited

Choose a reason for hiding this comment

marcopiraccini Jul 26, 2023 • edited

Choose a reason for hiding this comment

mcollina commented Jul 28, 2023

mcollina left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

marcopiraccini commented Jul 20, 2023 •

edited

marcopiraccini Jul 26, 2023 •

edited

marcopiraccini Jul 26, 2023 •

edited