Add Google Pubsub exporter for OTLP messages - PR1 #3551

alexvanboxel · 2021-05-26T21:56:56Z

Description:
Add the skeleton for Google Pubsub exporter for OTLP messages. The export is able to send OTEP messages to a Google Pubsub topic.

This exporter is the 2nd rewrite following feedback from the community on the original PR ( #1892 ), and also implements feedback given in the OTLP/Messaging otep:
open-telemetry/oteps#157

PR1 as described in the CONTRIBUTING guide only the README, config, and factory.

Link to tracking Issue:
#1802

Testing:
Tests for config and factory

Documentation:
Up-to-date README

bogdandrutu · 2021-05-27T16:06:48Z

/cc @punya @dashpole

bogdandrutu

Add CODEOWNERS entry, re-run make gendependabot

alexvanboxel · 2021-05-28T20:36:40Z

Add CODEOWNERS entry, re-run make gendependabot

I'm confused, do I need to add myself to the CODEOWNERS? or add @punya @dashpole as codeowners... I'm happy to set myself, but it feels weird to set myself for the initial reviews. If it's created up, I'll add the remarks to the CONTRIBUTING files (as it's not clear to me)

alexvanboxel · 2021-05-29T11:14:44Z

Rebased, squached and added me to code owners, and added ependabot

bogdandrutu · 2021-06-01T14:38:40Z

I would like @punya @dashpole and you to decide on who is going to maintain this.

alexvanboxel · 2021-06-01T14:45:20Z

We're using this within our company (Collibra) for about half a year now, and it's been battle-tested. Bumping almost every release. The plan is to keep it up-to-date. (as well as: #3552 ).

This would also be a testing ground from : open-telemetry/oteps#157

punya · 2021-06-01T15:13:43Z

@alexvanboxel, thank you for building, testing and contributing this!

We (GCP) would prefer if this exporter continues to be maintained by @alexvanboxel and the OTel community. I'll add a comment to the README suggesting a notice that clarifies ownership and support expectations.

exporter/googlecloudpubsubexporter/README.md

alexvanboxel · 2021-06-01T17:33:48Z

Comments about community module added, note that following the guidelines the implementation and the switch are upcoming
/cc @punya @bogdandrutu

bogdandrutu

@punya please still review :)

bogdandrutu · 2021-06-02T05:01:07Z

exporter/googlecloudpubsubexporter/config.go

+	// Override of the Pubsub endpoint
+	Endpoint string `mapstructure:"endpoint"`
+	// Only has effect if Endpoint is not ""
+	UseInsecure bool `mapstructure:"use_insecure"`


Be consistent with other places where we use insecure https://github.com/open-telemetry/opentelemetry-collector/blob/1f7c6618070197a7818371e747de4e0d61cbc9d9/config/configtls/configtls.go#L54

Tried to be consistent with the Google exporter:

opentelemetry-collector-contrib/exporter/googlecloudexporter/config.go

Line 30 in aa32f1e

UseInsecure bool `mapstructure:"use_insecure"`

But switch to insecure

bogdandrutu · 2021-06-02T05:01:57Z

exporter/googlecloudpubsubexporter/exporter.go

+func (ex *pubsubExporter) ConsumeTraces(ctx context.Context, td pdata.Traces) error {
+	return nil
+}
+
+func (ex *pubsubExporter) ConsumeMetrics(ctx context.Context, td pdata.Metrics) error {
+	return nil
+}
+
+func (ex *pubsubExporter) ConsumeLogs(ctx context.Context, td pdata.Logs) error {
+	return nil
+}


They don't need to be "exported" since you use only reference can be called pushTraces.

Changed, as well as shutdown/start

exporter/googlecloudpubsubexporter/README.md

exporter/googlecloudpubsubexporter/config.go

exporter/googlecloudpubsubexporter/README.md

punya · 2021-06-02T16:43:27Z

exporter/googlecloudpubsubexporter/exporter.go

+
+const name = "googlecloudpubsub"
+
+// pubsubExporter is a wrapper struct of OT cloud trace exporter


Is this comment accurate? I don't see any dependencies on the OT cloud trace exporter, will they be introduced in a future PR?

Oeps, removed comment. Old comment, from the start of the implementation

punya · 2021-06-02T16:46:19Z

exporter/googlecloudpubsubexporter/factory.go

+		exporterhelper.WithLogs(createLogsExporter))
+}
+
+var exporters = map[config.Exporter]*pubsubExporter{}


Could you add some comments explaining why this global mutable state is necessary? Based on a spot check, I didn't see such a pattern used in other exporters.

A lot of exporter/receivers have other endpoints to send data to/from. Here everything is piped over a single pubsub topic/subscription so there is a need to reuse the same connection.

Suggested change

var exporters = map[config.Exporter]*pubsubExporter{}

// This map allows multiple exporter instances to be multiplexed over the same PubSub connection.

// It is initialized at process startup and read-only afterwards, so we don't need to guard it for concurrent access.

var exporters = map[*Config]*pubsubExporter{}

I also suggest changing the map key type to the concrete type that we care about rather than an interface, because that documents the intent better.

We don't have this for exporters, only problem is for components that need to share the same port/endpoint across traces/logs/metrics, so don't think you need this.

It originally was the concrete type but making it the interface removed the need to cast at that point

Yes, the exporter joins the same topic, so the same gRPC connection to Pubsub

Note: this rule of splitting a PR into different phases makes it really annoying to contribute a new module. I have the full implementation working for more than half a year, but now have to retrofit all the comments back in that implementation hoping I don't do mistakes, and then merge that back for PR2 (the implementation). Having the implementation already available at the initial commit would make reviewing also easier. Just my 2 cents.

alexvanboxel · 2021-06-11T15:20:19Z

Rebased to the latest main and squashed everything as a lot of changes were required. I think all concerns in the comments are addressed.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

alexvanboxel requested a review from a team May 26, 2021 21:56

github-actions bot assigned jrcamp May 26, 2021

alexvanboxel force-pushed the feature/googlecloudpubsubexporter-pr1 branch 2 times, most recently from 9a3779a to 3e62e58 Compare May 27, 2021 07:22

bogdandrutu reviewed May 27, 2021

View reviewed changes

alexvanboxel force-pushed the feature/googlecloudpubsubexporter-pr1 branch from 3e62e58 to bda55f2 Compare May 28, 2021 05:35

alexvanboxel force-pushed the feature/googlecloudpubsubexporter-pr1 branch from bda55f2 to a9a9638 Compare May 29, 2021 11:05

punya reviewed Jun 1, 2021

View reviewed changes

exporter/googlecloudpubsubexporter/README.md Show resolved Hide resolved

bogdandrutu reviewed Jun 2, 2021

View reviewed changes

punya reviewed Jun 2, 2021

View reviewed changes

punya mentioned this pull request Jun 2, 2021

Add Google Pubsub receiver for OTLP messages - PR1 #3552

Merged

alexvanboxel force-pushed the feature/googlecloudpubsubexporter-pr1 branch from a58f06e to 14a459b Compare June 2, 2021 21:22

alexvanboxel requested review from punya and bogdandrutu June 3, 2021 05:08

alexvanboxel force-pushed the feature/googlecloudpubsubexporter-pr1 branch 2 times, most recently from 81ab5c4 to 3099356 Compare June 11, 2021 12:45

Add Google Pubsub exporter for OTLP messages

631a67d

alexvanboxel force-pushed the feature/googlecloudpubsubexporter-pr1 branch from 3099356 to 631a67d Compare June 14, 2021 08:36

bogdandrutu approved these changes Jun 14, 2021

View reviewed changes

bogdandrutu merged commit c9c8f1d into open-telemetry:main Jun 14, 2021

alexperez52 referenced this pull request in open-o11y/opentelemetry-collector-contrib Aug 18, 2021

Change LogRecordsCount to LogRecordCount everywhere (#3551)

32c2d0f

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>

alexvanboxel mentioned this pull request Sep 15, 2023

REQUEST: New membership for @alexvanboxel open-telemetry/community#1681

Closed

6 tasks

alexvanboxel deleted the feature/googlecloudpubsubexporter-pr1 branch November 16, 2023 06:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Google Pubsub exporter for OTLP messages - PR1 #3551

Add Google Pubsub exporter for OTLP messages - PR1 #3551

alexvanboxel commented May 26, 2021

bogdandrutu commented May 27, 2021

bogdandrutu left a comment

alexvanboxel commented May 28, 2021

alexvanboxel commented May 29, 2021

bogdandrutu commented Jun 1, 2021

alexvanboxel commented Jun 1, 2021

punya commented Jun 1, 2021

alexvanboxel commented Jun 1, 2021

bogdandrutu left a comment

bogdandrutu Jun 2, 2021

alexvanboxel Jun 2, 2021

bogdandrutu Jun 2, 2021

alexvanboxel Jun 2, 2021

punya Jun 2, 2021

alexvanboxel Jun 2, 2021

punya Jun 2, 2021

alexvanboxel Jun 2, 2021

punya Jun 3, 2021

bogdandrutu Jun 3, 2021

alexvanboxel Jun 4, 2021

alexvanboxel commented Jun 11, 2021


		const name = "googlecloudpubsub"

		// pubsubExporter is a wrapper struct of OT cloud trace exporter

-var exporters = map[config.Exporter]*pubsubExporter{}
+// This map allows multiple exporter instances to be multiplexed over the same PubSub connection.
+// It is initialized at process startup and read-only afterwards, so we don't need to guard it for concurrent access.
+var exporters = map[*Config]*pubsubExporter{}

Add Google Pubsub exporter for OTLP messages - PR1 #3551

Add Google Pubsub exporter for OTLP messages - PR1 #3551

Conversation

alexvanboxel commented May 26, 2021

bogdandrutu commented May 27, 2021

bogdandrutu left a comment

Choose a reason for hiding this comment

alexvanboxel commented May 28, 2021

alexvanboxel commented May 29, 2021

bogdandrutu commented Jun 1, 2021

alexvanboxel commented Jun 1, 2021

punya commented Jun 1, 2021

alexvanboxel commented Jun 1, 2021

bogdandrutu 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

alexvanboxel commented Jun 11, 2021