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

Add back the otlpmetric{http,grpc} exporter code #2807

Closed
9 tasks done
MrAlias opened this issue Apr 19, 2022 · 5 comments · Fixed by #3155
Closed
9 tasks done

Add back the otlpmetric{http,grpc} exporter code #2807

MrAlias opened this issue Apr 19, 2022 · 5 comments · Fixed by #3155
Assignees
Labels
area:metrics Part of OpenTelemetry Metrics pkg:exporter:otlp Related to the OTLP exporter package

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Apr 19, 2022

Blocked by the metric SDK having a working example in the new_sdk/main branch

Removed in #2802

@MrAlias MrAlias added area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package labels Apr 19, 2022
@MrAlias MrAlias added this to the Metric SDK: Alpha milestone Apr 19, 2022
@MrAlias MrAlias self-assigned this Aug 4, 2022
@MrAlias
Copy link
Contributor Author

MrAlias commented Aug 11, 2022

Looking at the previous package structure, I do not think the otlpmetric.Exporter or otlpmetric.Client types should be re-introduced. Having this exporter type that needs an additional concept of a client is not useful to the user. It only adds to the mental strain of setting up an exporter having to know that you need a client in addition to this exporter. And in reality, the end-user never interacts with this type. They just call New from a client implementation package (e.g. otlpmetricgrpc.New).

Instead of bringing this confusion forward into the next release I propose these types are removed from the otlpmetric package, the implementation details of a client are not exposed from the otlpmetrichttp or otlpmetricgrpc packages, and instead those packages each have the following New functions:

func New(ctx context.Context, opts ...Option) (metric.Exporter, error)

@MrAlias
Copy link
Contributor Author

MrAlias commented Aug 11, 2022

Now is a good time to reduce the options included in the otlpmetricgrpc package. We should accept a gRPC connection that the user creates and just use that. We should stop creating and trying to manage gRPC connections with the exporter. Doing so would mean we would have to track non-stable gRPC features with our stable exporter and continually try to keep up with adding the same features gRPC does.

@MrAlias
Copy link
Contributor Author

MrAlias commented Aug 11, 2022

Now is a good time to reduce the options included in the otlpmetricgrpc package. We should accept a gRPC connection that the user creates and just use that. We should stop creating and trying to manage gRPC connections with the exporter. Doing so would mean we would have to track non-stable gRPC features with our stable exporter and continually try to keep up with adding the same features gRPC does.

This might not be possible if we plan to continue support for environment variables. A connection will need to be setup in that case.

@MrAlias
Copy link
Contributor Author

MrAlias commented Aug 12, 2022

Looking at the previous package structure, I do not think the otlpmetric.Exporter or otlpmetric.Client types should be re-introduced. Having this exporter type that needs an additional concept of a client is not useful to the user. It only adds to the mental strain of setting up an exporter having to know that you need a client in addition to this exporter. And in reality, the end-user never interacts with this type. They just call New from a client implementation package (e.g. otlpmetricgrpc.New).

Instead of bringing this confusion forward into the next release I propose these types are removed from the otlpmetric package, the implementation details of a client are not exposed from the otlpmetrichttp or otlpmetricgrpc packages, and instead those packages each have the following New functions:

func New(ctx context.Context, opts ...Option) (metric.Exporter, error)

Giving this some thought, the need for consistency out-weighs the improvements this alternate approach would offer. Users already using the otlptracegrpc exporter would be confused by the difference more than appreciative is my guess. I think moving forward with the original design makes sense here.

@MrAlias
Copy link
Contributor Author

MrAlias commented Sep 12, 2022

Closed by #3155

@MrAlias MrAlias closed this as completed Sep 12, 2022
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 pkg:exporter:otlp Related to the OTLP exporter package
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

1 participant