obsservice: turn the ObsService into an OTLP server by andreimatei · Pull Request #95632 · cockroachdb/cockroach

andreimatei · 2023-01-20T22:45:13Z

This patch makes the Obs Service implement the OTLP Logs gRPC service.
This reverses the connection direction for exporting events out of CRDB:
now CRDB connects out to the Obs Service through a new --obsservice-addr
flag. The Obs Service gets a new --otlp-addr flag taking the
host:port on which to expose the service.

By using standard OTLP we can have the data flow through an
OpenTelemetry Collector before going to the Obs Service. This makes the
Obs Service topology more flexible (since we can rely on the collector
to be deployed widely and funnel the data), and gives the collector the
opportunity to tee the data to other destinations.

Release note: None
Epic: CRDB-23641

cockroach-teamcity · 2023-01-20T22:45:25Z

This change is

andreimatei · 2023-01-20T22:45:39Z

First commit is #93733

cucaroach · 2023-01-23T16:58:54Z

pkg/obsservice/obspb/update-opentelemetry-proto.sh

-find $DEST_DIR -type f -name "*.proto" -exec sed -i "s/option go_package = \"go.opentelemetry.io\/proto\/otlp\/.*\/v1\"/option go_package = \"v1\"/" {} + ;
+if [ `uname` = 'Darwin' ]; then
+  # for OSX
+  find $DEST_DIR -type f -name "*.proto" -exec sed -i '' -e "s/option go_package = \"go.opentelemetry.io\/proto\/otlp\/.*\/v1\"/option go_package = \"v1\"/" {} + ;


/ is perhaps not the best choice for sed separator here, I'd use |

andreimatei

First commit is #93733

Now all commits are original.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aadityasondhi, @abarganier, @cucaroach, @dhartunian, @herkolategan, and @srosenberg)

pkg/obsservice/obspb/update-opentelemetry-proto.sh line 85 at r3 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

/ is perhaps not the best choice for sed separator here, I'd use |

Ack, This change is in #93733, which merged. I'll probably leave it alone.

dhartunian

just some minor comments/nits. excited to see where this leads!

Reviewed 6 of 27 files at r5, 9 of 15 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aadityasondhi, @abarganier, @andreimatei, and @cucaroach)

pkg/base/test_server_args.go line 170 at r6 (raw file):

	// ObsServiceAddr is the address to which events will be exported over OTLP.
	// If empty, exporting events is inhibited.
	ObsServiceAddr string

Minor: I'm wondering if this should also be an OTLPSinkAddr or something. could encourage folks to integrate with their own infra. Either way, maybe best not to encourage that just yet.

pkg/obs/event_exporter.go line 251 at r6 (raw file):

			select {
			case <-ctx.Done():
				//	// We'll return after flushing everything.

nit :extra //

pkg/obsservice/obslib/ingest/ingest.go line 45 at r6 (raw file):

) (*logspb.ExportLogsServiceResponse, error) {
	_ = persistEvents(ctx, request.ResourceLogs, e.db)
	return &logspb.ExportLogsServiceResponse{}, nil

why don't we care about the error here?

pkg/server/tenant.go line 1017 at r6 (raw file):

	remoteFlowRunner := flowinfra.NewRemoteFlowRunner(baseCfg.AmbientCtx, stopper, &remoteFlowRunnerAcc)

	// Create the EventServer. It will be made operational later, after the

nit: update comment.

pkg/sql/event_log.go line 699 at r6 (raw file):

		// others serialize their UUIDs, but I went with the network byte order.
		binary.BigEndian.PutUint64(traceID[:], uint64(sp.TraceID()))
		binary.BigEndian.PutUint64(spanID[:], uint64(sp.SpanID()))

does this change have impact on existing deployments? I guess these are random IDs anyway, but just want to check. I don't think we read these back anywhere.

abarganier

Very cool - modulo a couple minor nits.

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @aadityasondhi, @andreimatei, and @cucaroach)

pkg/obs/event_exporter.go line 276 at r6 (raw file):

				s.buf.mu.sizeBytes -= sizeBytes
				msgSize += sizeBytes
				req.ResourceLogs[0].ScopeLogs = append(req.ResourceLogs[0].ScopeLogs,

nit: would it make sense to pre-allocate ScopeLogs to len(s.buf.mu.events)?

Code quote:

.ScopeLogs

pkg/obsservice/obslib/ingest/ingest.go line 35 at r6 (raw file):

// SetDB sets the database connection that the ingester will use to persist the
// events it receives. It needs to be called before Export().

nit: should we check the e.db is defined in Export() before calling persistEvents()? Seems like we might hit an NPE panic otherwise

Code quote:

It needs to be called before Export().

andreimatei

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @aadityasondhi, @cucaroach, and @dhartunian)

pkg/base/test_server_args.go line 170 at r6 (raw file):

Previously, dhartunian (David Hartunian) wrote…

Minor: I'm wondering if this should also be an OTLPSinkAddr or something. could encourage folks to integrate with their own infra. Either way, maybe best not to encourage that just yet.

yeah... I figured that putting ObsService in these names is more suggestive about what this data is about.
If anything, I think the name of the flag that's externally visible is more debatable... But I dunno, I'd keep "obs service" for now at least.

pkg/obs/event_exporter.go line 251 at r6 (raw file):

Previously, dhartunian (David Hartunian) wrote…

nit :extra //

done

pkg/obs/event_exporter.go line 276 at r6 (raw file):

Previously, abarganier (Alex Barganier) wrote…

nit: would it make sense to pre-allocate ScopeLogs to len(s.buf.mu.events)?

done

pkg/obsservice/obslib/ingest/ingest.go line 35 at r6 (raw file):

Previously, abarganier (Alex Barganier) wrote…

nit: should we check the e.db is defined in Export() before calling persistEvents()? Seems like we might hit an NPE panic otherwise

done

pkg/obsservice/obslib/ingest/ingest.go line 45 at r6 (raw file):

Previously, dhartunian (David Hartunian) wrote…

why don't we care about the error here?

added logging
As far as returning it, you're not really supposed to return errors from this API according to OTLP.

pkg/server/tenant.go line 1017 at r6 (raw file):

Previously, dhartunian (David Hartunian) wrote…

nit: update comment.

done

pkg/sql/event_log.go line 699 at r6 (raw file):

Previously, dhartunian (David Hartunian) wrote…

does this change have impact on existing deployments? I guess these are random IDs anyway, but just want to check. I don't think we read these back anywhere.

Nobody cares about these so far.

This patch switches the main obsservice cobra command to return errors instead of panicking internally. This is nicer since Cobra prints the error. Release note: None Epic: None

This patch makes the Obs Service implement the OTLP Logs gRPC service. This reverses the connection direction for exporting events out of CRDB: now CRDB connects out to the Obs Service through a new --obsservice-addr flag. The Obs Service gets a new `--otlp-addr` flag taking the host:port on which to expose the service. By using standard OTLP we can have the data flow through an OpenTelemetry Collector before going to the Obs Service. This makes the Obs Service topology more flexible (since we can rely on the collector to be deployed widely and funnel the data), and gives the collector the opportunity to tee the data to other destinations. Release note: None Epic: CRDB-16791 Fixes: CRDB-23641

andreimatei · 2023-02-07T22:40:02Z

bors r+

craig · 2023-02-07T23:50:57Z

Build succeeded:

Bazel Essential CI (Cockroach)

andreimatei requested review from a team, aadityasondhi, abarganier and dhartunian January 20, 2023 22:45

andreimatei requested review from a team as code owners January 20, 2023 22:45

andreimatei requested review from cucaroach, herkolategan and srosenberg and removed request for a team January 20, 2023 22:45

andreimatei force-pushed the obsservice.otlp branch from d729e5e to 8c869da Compare January 21, 2023 22:35

andreimatei changed the title ~~obsservice: add logs_service proto for OTLP collector~~ obsservice: turn the ObsService into an OTLP server Jan 22, 2023

andreimatei force-pushed the obsservice.otlp branch 2 times, most recently from db59d4d to 6780ed0 Compare January 22, 2023 19:23

cucaroach reviewed Jan 23, 2023

View reviewed changes

andreimatei force-pushed the obsservice.otlp branch from 6780ed0 to 5695b61 Compare January 23, 2023 20:27

andreimatei commented Jan 23, 2023

View reviewed changes

andreimatei mentioned this pull request Jan 23, 2023

obsservice: support embedding in CRDB nodes #95703

Merged

andreimatei removed request for herkolategan and srosenberg January 24, 2023 00:08

andreimatei force-pushed the obsservice.otlp branch 2 times, most recently from 273d4c2 to 047724d Compare February 1, 2023 00:44

dhartunian approved these changes Feb 1, 2023

View reviewed changes

abarganier approved these changes Feb 1, 2023

View reviewed changes

andreimatei force-pushed the obsservice.otlp branch from 047724d to 6c47736 Compare February 6, 2023 21:46

andreimatei commented Feb 6, 2023

View reviewed changes

andreimatei added 2 commits February 7, 2023 14:14

obsservice: switch to returning errors from cobra cmd

4153df9

This patch switches the main obsservice cobra command to return errors instead of panicking internally. This is nicer since Cobra prints the error. Release note: None Epic: None

andreimatei force-pushed the obsservice.otlp branch from 6c47736 to cad0546 Compare February 7, 2023 20:49

craig bot merged commit 247a543 into cockroachdb:master Feb 7, 2023

andreimatei deleted the obsservice.otlp branch February 22, 2023 20:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

obsservice: turn the ObsService into an OTLP server #95632

obsservice: turn the ObsService into an OTLP server #95632
craig[bot] merged 2 commits intocockroachdb:masterfrom
andreimatei:obsservice.otlp

andreimatei commented Jan 20, 2023 •

edited

Loading

Uh oh!

cockroach-teamcity commented Jan 20, 2023

Uh oh!

andreimatei commented Jan 20, 2023

Uh oh!

cucaroach Jan 23, 2023

Uh oh!

andreimatei left a comment

Uh oh!

dhartunian left a comment

Uh oh!

abarganier left a comment

Uh oh!

andreimatei left a comment

Uh oh!

andreimatei commented Feb 7, 2023

Uh oh!

craig bot commented Feb 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

andreimatei commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Jan 20, 2023

Uh oh!

andreimatei commented Jan 20, 2023

Uh oh!

cucaroach Jan 23, 2023

Choose a reason for hiding this comment

Uh oh!

andreimatei left a comment

Choose a reason for hiding this comment

Uh oh!

dhartunian left a comment

Choose a reason for hiding this comment

Uh oh!

abarganier left a comment

Choose a reason for hiding this comment

Uh oh!

andreimatei left a comment

Choose a reason for hiding this comment

Uh oh!

andreimatei commented Feb 7, 2023

Uh oh!

craig bot commented Feb 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

andreimatei commented Jan 20, 2023 •

edited

Loading