Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Telegraf/InfluxDB exporter #2952

Merged
merged 8 commits into from
May 4, 2021
Merged

Telegraf/InfluxDB exporter #2952

merged 8 commits into from
May 4, 2021

Conversation

jacobmarble
Copy link
Contributor

Description: Add influxdbexporter (WIP)

This change adds an exporter for InfluxData line protocol, the native wire transfer protocol for Telegraf and InfluxDB.

The conversion logic lives in package otel2influx, and is also imported by the proposed Telegraf OpenTelemetry input plugin. The objective of this strategy is to create a canonical mapping, and an open-source implementation, of OpenTelemetry <-> Telegraf/InfluxDB translation.

I am not proposing that otel2influx, in its current state, be the canonical mapping. Let's discuss that topic separate from this exporter.

One shortcoming of this approach is that both otel2influx and opentelemetry-collector package their own generated protocol buffer objects. This means that every OTLP protobuf object is serialized and deserialized in this exporter. I propose that opentelemetry-collector instead depend on opentelemetry-proto-go, for the benefit of this exporter and other third-party packages.

// r is a protobuf from otel2influx
// td is a protobuf wrapper from otelcol
var r otlpcollectortrace.ExportTraceServiceRequest
if protoBytes, err := td.ToOtlpProtoBytes(); err != nil {
	return err
} else if err = proto.Unmarshal(protoBytes, &r); err != nil {
	return err
}

Link to tracking Issue: #2951

Testing: Little testing so far. The translation logic should be tested in package otel2influx (but is not currently).

Documentation: README.md included in new exporter.

@jacobmarble jacobmarble requested a review from a team March 31, 2021 22:33
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 31, 2021

CLA Signed

The committers are authorized under a signed CLA.

  • ✅ Jeff Cheng (7ac7a9756eba399accd045b5a45980c34a7c81ac)

@jacobmarble jacobmarble changed the title Telegraf/InfluxDB output plugin Telegraf/InfluxDB exporter Mar 31, 2021
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 7, 2021

CLA Signed

The committers are authorized under a signed CLA.

  • ✅ Jacob Marble (7edc114d679800cf126b4226ec3778d547a39a71)

@codecov
Copy link

codecov bot commented Apr 7, 2021

Codecov Report

Merging #2952 (86adb77) into main (17399fa) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 86adb77 differs from pull request most recent head db9726e. Consider uploading reports for the commit db9726e to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2952   +/-   ##
=======================================
  Coverage   91.91%   91.91%           
=======================================
  Files         494      493    -1     
  Lines       23939    23947    +8     
=======================================
+ Hits        22003    22011    +8     
  Misses       1429     1429           
  Partials      507      507           
Flag Coverage Δ
integration 63.32% <0.00%> (-0.37%) ⬇️
unit 90.93% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/otelcontribcol/components.go 88.50% <100.00%> (+0.13%) ⬆️
exporter/signalfxexporter/config.go 83.67% <0.00%> (-2.38%) ⬇️
exporter/humioexporter/humio_client.go 94.36% <0.00%> (-0.08%) ⬇️
exporter/awsemfexporter/util.go 100.00% <0.00%> (ø)
receiver/redisreceiver/pdata.go 100.00% <0.00%> (ø)
exporter/humioexporter/config.go 100.00% <0.00%> (ø)
exporter/humioexporter/factory.go 100.00% <0.00%> (ø)
exporter/splunkhecexporter/client.go 93.75% <0.00%> (ø)
pkg/batchpersignal/batchpersignal.go 100.00% <0.00%> (ø)
receiver/redisreceiver/redis_metric.go 100.00% <0.00%> (ø)
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84952e2...db9726e. Read the comment docs.

@tigrannajaryan
Copy link
Member

@jacobmarble thanks for the contribution, this is very welcome!

I propose that opentelemetry-collector instead depend on opentelemetry-proto-go, for the benefit of this exporter and other third-party packages.

Unfortunately there is no easy way to do that. Collector uses Gogo Protobuf library (for performance reasons). opentelemetry-proto-go uses canonical Go Protobuf library. The in-memory representations of these 2 are different and incompatible.

@jacobmarble
Copy link
Contributor Author

@jacobmarble thanks for the contribution, this is very welcome!

I propose that opentelemetry-collector instead depend on opentelemetry-proto-go, for the benefit of this exporter and other third-party packages.

Unfortunately there is no easy way to do that. Collector uses Gogo Protobuf library (for performance reasons). opentelemetry-proto-go uses canonical Go Protobuf library. The in-memory representations of these 2 are different and incompatible.

@tigrannajaryan I wanted to bring this up at the Golang SIG earlier today. Are you aware of a plan to consolidate?

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR, thanks.
I am only blocking until we clarify the double-registration of Protobuf messages.

exporter/influxdbexporter/README.md Outdated Show resolved Hide resolved
exporter/influxdbexporter/README.md Outdated Show resolved Hide resolved
InfluxDB/IOx is an open-source time series storage engine, capable of storing and querying large (and small) time series datasets.
The schema used by this OpenTelemetry exporter is optimized for InfluxDB/IOx performance.

### Definitions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice writeup. I wonder if it should be extracted and moved elsewhere (can be somewhere in this repo) since it is not Influxdb-specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it doesn't belong here. My original audience for this section was other Influx employees, not the OpenTelemetry community.

The original is copied out to a text file here. Can you suggest a specific destination for it?

exporter/influxdbexporter/README.md Outdated Show resolved Hide resolved
exporter/influxdbexporter/README.md Outdated Show resolved Hide resolved
exporter/influxdbexporter/README.md Outdated Show resolved Hide resolved
exporter/influxdbexporter/config.go Show resolved Hide resolved
exporter/influxdbexporter/exporter.go Outdated Show resolved Hide resolved
exporter/influxdbexporter/exporter.go Outdated Show resolved Hide resolved
exporter/influxdbexporter/writer.go Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

@jacobmarble thanks for the contribution, this is very welcome!

I propose that opentelemetry-collector instead depend on opentelemetry-proto-go, for the benefit of this exporter and other third-party packages.

Unfortunately there is no easy way to do that. Collector uses Gogo Protobuf library (for performance reasons). opentelemetry-proto-go uses canonical Go Protobuf library. The in-memory representations of these 2 are different and incompatible.

@tigrannajaryan I wanted to bring this up at the Golang SIG earlier today. Are you aware of a plan to consolidate?

No existing plans AFAIK, but we likely want to do something about it anyway, since it was recently reported that Gogo Protobufs are no longer maintained. Good topic for the next Collector or Go SIG.

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@jacobmarble
Copy link
Contributor Author

(not stale)

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@jacobmarble
Copy link
Contributor Author

FYI the corresponding Telegraf OpenTelemetry input plugin has been merged.
influxdata/telegraf#9077

@jacobmarble
Copy link
Contributor Author

jacobmarble commented May 3, 2021

@tigrannajaryan FYI here is the corresponding receiver diff. Since there may be feedback here that improves that change, I'll wait to create that PR.

exporter/influxdbexporter/exporter.go Outdated Show resolved Hide resolved
exporter/influxdbexporter/exporter.go Outdated Show resolved Hide resolved
exporter/influxdbexporter/exporter.go Outdated Show resolved Hide resolved
exporter/influxdbexporter/exporter.go Outdated Show resolved Hide resolved
exporter/influxdbexporter/factory.go Outdated Show resolved Hide resolved
exporter/influxdbexporter/writer.go Show resolved Hide resolved
exporter/influxdbexporter/writer.go Outdated Show resolved Hide resolved
exporter/influxdbexporter/writer.go Show resolved Hide resolved
exporter/influxdbexporter/writer.go Outdated Show resolved Hide resolved
@tigrannajaryan tigrannajaryan self-requested a review May 4, 2021 14:18
@tigrannajaryan
Copy link
Member

@jacobmarble Mostly LGTM, thank you. I left a few minor comments.

@tigrannajaryan
Copy link
Member

@tigrannajaryan FYI here is the corresponding receiver diff. Since there may be feedback here that improves that change, I'll wait to create that PR.

I glanced at the receiver and it appears to be doing the mirror of this exporter, which looks reasonable.

@jacobmarble
Copy link
Contributor Author

@jacobmarble Mostly LGTM, thank you. I left a few minor comments.

@tigrannajaryan thanks for the thorough review. Clearly this PR has collected some dust over the last few weeks.

@tigrannajaryan tigrannajaryan merged commit 4cb6765 into open-telemetry:main May 4, 2021
@jacobmarble jacobmarble deleted the influxdb branch May 4, 2021 17:06
alexperez52 referenced this pull request in open-o11y/opentelemetry-collector-contrib Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants