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

Do NOT expose generated protos in metrics exporter #3455

Closed
bogdandrutu opened this issue Nov 9, 2022 · 4 comments · Fixed by #3486
Closed

Do NOT expose generated protos in metrics exporter #3455

bogdandrutu opened this issue Nov 9, 2022 · 4 comments · Fixed by #3486
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics bug Something isn't working help wanted Extra attention is needed pkg:exporter:otlp Related to the OTLP exporter package

Comments

@bogdandrutu
Copy link
Member

See https://github.com/open-telemetry/opentelemetry-go/blob/main/exporters/otlp/otlpmetric/client.go and just remember your issue from 6 months (changes can happen in the generated code that are not backwards compatible from golang perspective).

@bogdandrutu bogdandrutu added the bug Something isn't working label Nov 9, 2022
@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics pkg:exporter:otlp Related to the OTLP exporter package and removed pkg:SDK Related to an SDK package labels Nov 9, 2022
@MrAlias
Copy link
Contributor

MrAlias commented Nov 10, 2022

Notes from SIG meeting today:

  • We should pull the client and exporter interface from otlpmetric into an internal pacakge. The grpc and http pacakges both already provide New(...) metric.Exporter, error functions that only wrap the otlpmetric logic.
    • This would create an asymmetry with the trace exporters
      • This may be warranted if it fixes a design issue in the trace setup
      • Moving to an internal package is a two-way door. We can add it back if users ask for the symmetry in the future.

@jmacd
Copy link
Contributor

jmacd commented Nov 15, 2022

IMO it makes sense to support a NewExporter() method which returns a public/exporter Exporter which uses the metricdata transfer objects that are under this repository's control. Then, remove the Client API which uses the generated protobuf objects. It's the Client that's the problem, not the Exporter.

@jmacd
Copy link
Contributor

jmacd commented Nov 15, 2022

As a test case for this, note that the Lightstep metrics SDK has been using the OTel-Go SDK's exporter for a while. I admit this was convenient for me, and since the OTel-Proto API was using generated code from the current OTLP, I could use the OTel-Go SDK's Exporter to export exponential histogram data, even though the SDK didn't support expo-histos.

If the Client API were removed that speak public OTLP protoc-generated APIs, the Lightstep metrics SDK will be blocked from updating until metricdata supports exponential histogram, at which time I would be glad to move forward to using the Exporter w/ metricdata (or an equivalent based on OTC's exporter/otlpexporter w/ pdata). Speaking as a consumer of SDK components, I do not feel I have a "Right" to the Client API that speaks public OTLP protoc-generated APIs.

@jmacd
Copy link
Contributor

jmacd commented Nov 21, 2022

Note that there is no current API to obtain a Client. The Exporter object keeps its Client private and the New() function in each of the sub-packages returns the Exporter.

So, I would say this issue is already solved for metrics. It remains to be seen whether there will be a breaking API change for the trace SDK, one where Client becomes internal and the NewClient() Client function is replaced by New() Exporter the way that has been done for metrics. I support this change, but it would seem to call for a major version bump.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics bug Something isn't working help wanted Extra attention is needed pkg:exporter:otlp Related to the OTLP exporter package
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants