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

OpenTelemetry Input Plugin #9077

Merged
merged 8 commits into from
Apr 30, 2021
Merged

OpenTelemetry Input Plugin #9077

merged 8 commits into from
Apr 30, 2021

Conversation

jacobmarble
Copy link
Member

@jacobmarble jacobmarble commented Mar 31, 2021

  • Updated associated README.md.
  • Wrote appropriate unit tests.

Related:

This change adds an input plugin opentelemetry_listener, which opens a gRPC listener to ingest OTLP signals.

OpenTelemetry is an open-source telemetry ecosystem, supporting traces, metrics and logs. The mapping of the OpenTelemetry data model to line protocol is a work-in-progress, and I welcome any feedback to that document.

The core conversion logic lives in influxdata/influxdb-observability/otel2influx, intended to stand as a canonical implementation of OTel->line protocol. That package is also imported by the above-mentioned PR "OTel Collector exporter..."

Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@telegraf-tiger telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin new plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Mar 31, 2021
@sjwang90
Copy link
Contributor

sjwang90 commented Apr 2, 2021

@jacobmarble Is this plugin blocked on the PRs you linked that you opened in the OpenTelemetry repo?

@jacobmarble
Copy link
Member Author

@jacobmarble Is this plugin blocked on the PRs you linked that you opened in the OpenTelemetry repo?

Not blocked, ready for your review.

Copy link
Contributor

@Hipska Hipska left a comment

Choose a reason for hiding this comment

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

Hi, looks good at first glance. Only question is, why is it called open telemetry_listener instead of just opentelemetry?

Also, a new plugin should have tests. Oh, and also check the current failing checks, it seems you will need to do a make fmt.

Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thanks @jacobmarble for submitting this PR! I have some comments in the code. Nothing big, but I agree with @Hipska that we need unit tests for this plugin!

Please keep the plugin name. @Hipska: all plugins passively waiting for the other side to connect and not actively gathering the metric have the _listener suffix, so let's keep it that way.

@srebhan srebhan self-assigned this Apr 19, 2021
@Hipska
Copy link
Contributor

Hipska commented Apr 19, 2021

Please keep the plugin name. @Hipska: all plugins passively waiting for the other side to connect and not actively gathering the metric have the _listener suffix, so let's keep it that way.

Oh, okay that is a valid point. Yes keep the name.

Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

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

all looks good. left a few comments for you to consider.
looks like you have to add github.com/influxdata/influxdb-observability to LICENSE_OF_DEPENDENCIES.md to resolve the build.

@jacobmarble jacobmarble requested a review from ssoroka April 29, 2021 22:00
@ssoroka ssoroka merged commit 6977121 into influxdata:master Apr 30, 2021
@jacobmarble jacobmarble deleted the jgm-opentelemetry branch April 30, 2021 14:39
@jacobmarble jacobmarble mentioned this pull request May 2, 2021
2 tasks
tigrannajaryan pushed a commit to open-telemetry/opentelemetry-collector-contrib that referenced this pull request May 4, 2021
This change adds an exporter for [InfluxData line protocol](https://docs.influxdata.com/influxdb/v2.0/reference/syntax/line-protocol/), the native wire transfer protocol for [Telegraf](https://www.influxdata.com/time-series-platform/telegraf/) and [InfluxDB](https://www.influxdata.com/products/influxdb/).

The conversion logic lives in [package `otel2influx`](https://github.com/influxdata/influxdb-observability/tree/main/otel2influx), and is also imported by the [proposed Telegraf OpenTelemetry input plugin](influxdata/telegraf#9077). 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.
```golang
// 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`](https://github.com/influxdata/influxdb-observability/tree/main/otel2influx) (but is not currently).

**Documentation:** README.md included in new exporter.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin new plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants