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

Remove unstable metrics dependency from the OTLP exporter #1827

Closed
MrAlias opened this issue Apr 20, 2021 · 10 comments
Closed

Remove unstable metrics dependency from the OTLP exporter #1827

MrAlias opened this issue Apr 20, 2021 · 10 comments
Assignees
Labels
pkg:exporter:otlp Related to the OTLP exporter package
Milestone

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Apr 20, 2021

In order to release the OTLP exporter in our RC and subsequent 1.0 release it needs to not depend on any packages that will be experimental. Currently the exporter depends on the metirc, sdk/metric, and sdk/export/metric packages:

go.opentelemetry.io/otel/metric v0.19.0
go.opentelemetry.io/otel/sdk v0.19.0
go.opentelemetry.io/otel/sdk/export/metric v0.19.0
go.opentelemetry.io/otel/sdk/metric v0.19.0

We need to find a way to factor these dependencies out while.

Proposed Solutions

  1. Remove all unstable metric support from the exporter. In the metrics packages add a wrapper of the OTLP exporter there that would extend the exporter to support metrics
  2. Build a "plugin" or "signal driver" model for the exporter. We would include a stable traces plugin and the metrics package would provide it's own plugin to support metircs.
@MrAlias MrAlias added release:1.0.0-rc.1 pkg:exporter:otlp Related to the OTLP exporter package labels Apr 20, 2021
@MrAlias MrAlias added this to the RC1 milestone Apr 20, 2021
@MrAlias MrAlias added this to To do in OpenTelemetry Go RC via automation Apr 20, 2021
@MrAlias MrAlias assigned paivagustavo and unassigned jmacd Apr 29, 2021
@paivagustavo paivagustavo moved this from To do to In progress in OpenTelemetry Go RC May 3, 2021
@jmacd
Copy link
Contributor

jmacd commented May 3, 2021

👀

@paivagustavo
Copy link
Member

My ideia to remove the metrics dependency is to split the current exporters by signal and protocol.

Creating 4 new client modules for gRPC metrics and traces, and HTTP metrics and traces. These clients would be passed to an Exporter that would wrap them and implement the sdk exporter interfaces.

otlp/
. grpc/
. . metrics/    module with gRPC metrics client
. . traces/     module with gRPC traces client
. http/
. . metrics/    module with HTTP metrics client
. . traces/     module with HTTP traces client
. metrics/      module with Exporter wrapper for clients
. traces/       module with Exporter wrapper for clients

Proposal usage example:

// Unstable Metrics exporters
otlpmetrics.NewExporter(otlpgrpcmetrics.NewClient(clientOps...), exporterOpts...)
otlpmetrics.NewExporter(otlphttpmetrics.NewClient(clientOps...), exporterOpts...)

// Stable Traces Exporters
otlptraces.NewExporter(otlpgrpctraces.NewClient(clientOps...), exporterOpts...)
otlptraces.NewExporter(otlphttptraces.NewClient(clientOps...), exporterOpts...)

Pros:

  • Segmented OTLP exporter means that we can set different expectations between each modules.
  • OTLP Clients! This helps pushing the OTLP format to a broader number of projects.
  • We can create custom clients to the OTLP format (this enables the creation of a io.Writer sink or another custom sink for OTLP format, as discussed in the last SIG Meeting).

Cons:

  • Too many modules, hard to name and to keep all sync'd up with updates;
  • We loose the simplicity of create an Exporter and that could be use both for Traces/Metrics; (we could create another wrapper that receiver two Expoters and implements both trace and metrics exporter?)

reminder to self: proto-go also needs to be updated to have modules per signal.

@jmacd
Copy link
Contributor

jmacd commented May 4, 2021

I was considering the "Cons" section above and agree about there being a lot of modules.
I tried to find a better approach with fewer modules, and didn't like what I found.
(Let's not try to solve the dependency injection problem in Go.)

I believe there will be users with legitimate reasons to want only one signal, and your solution works for them. Even when both are stable, this organization will probably hold its value.

If we didn't have multiple dimensions of variation, this would be easier. The use of import _ "..." doesn't help as much in a case where there are multiple dependencies to inject. Surely there is a way to use underscore-imports, but I'm not sure it's better than what you proposed.

I support this plan.

@jmacd
Copy link
Contributor

jmacd commented May 4, 2021

We loose the simplicity of create an Exporter and that could be use both for Traces/Metrics

Is it good enough for these users to create one driver and share it between two exporters? The connection state will be shared -- there's not much other reason to combine the exporters, the data paths never cross.

@pellared
Copy link
Member

pellared commented May 25, 2021

. metrics/      module with Exporter wrapper for clients
. traces/       module with Exporter wrapper for clients

What do you think of making these modules internal so that should be wrapped? So that the client would not directly depend on this helper module.
More: #1922 (comment)

@MrAlias
Copy link
Contributor Author

MrAlias commented May 25, 2021

. metrics/      module with Exporter wrapper for clients
. traces/       module with Exporter wrapper for clients

What do you think of making these modules internal so that should be wrapped? So that the client would not directly depend on this helper module.
More: #1922 (comment)

Would this resolve this issue, would the metrics dependency be removed if this approach was taken?

I'm not sure I follow your suggestion here. Can you expand it?

@pellared
Copy link
Member

pellared commented May 25, 2021

It would NOT make it possible to remove the dependency without making breaking API changes.

This is the line I am concerned about. It is returning a struct for another module: *otlptrace.Exporter

@paivagustavo
Copy link
Member

paivagustavo commented May 25, 2021

It would NOT make it possible to remove the dependency without making breaking API changes.

This is the line I am concerned about. It is returning a struct for another module: *otlptrace.Exporter

Could you elaborate more about your concern? I think every exporter is doing something like that, even the current otlpexporter. Why is that a bad thing?

EDIT: Got your point now, as I commented in the #1922, we should probably discuss that more in this week sig about the keeping the otlptrace or making that internal.

@MrAlias
Copy link
Contributor Author

MrAlias commented Jun 17, 2021

Talking about this in the SIG meeting today, this looks "done enough" to release.

We should close this and capture the remaining tasks should moved to a new issue.

@MrAlias
Copy link
Contributor Author

MrAlias commented Jun 17, 2021

Remaining tasks that I'll open issues for:

  • add HTTP otlp metrics exporter
  • refactor out the duplicated code (transform, connection, otlpconfig)
  • update examples and remove old otlp gRPC/HTTP exporter
  • update docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:exporter:otlp Related to the OTLP exporter package
Projects
No open projects
Development

No branches or pull requests

4 participants