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

otlptracegrpc v2 #5248

Closed
pellared opened this issue Apr 22, 2024 · 9 comments
Closed

otlptracegrpc v2 #5248

pellared opened this issue Apr 22, 2024 · 9 comments
Labels
area:trace Part of OpenTelemetry tracing enhancement New feature or request pkg:exporter:otlp Related to the OTLP exporter package

Comments

@pellared
Copy link
Member

pellared commented Apr 22, 2024

The scope of this issue is to create a new otlptracegrpc v2 module which would have API similar to otlpmetricgrpc.

Unfortunately, it is not possible (without making a breaking change) to remove the dependency on google.golang.org/grpc in otlptracehttp because of https://pkg.go.dev/go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp#NewClient as https://pkg.go.dev/go.opentelemetry.io/otel/exporters/otlp/otlptrace#Client exposes the proto package which depends on google.golang.org/grpc.

The fix would require creating v2 modules. For oteltracehttp and oteltracegrpc. After these are created we still need to support the v1 modules for at least a year. I think for ease of review we should create the new v2 modules of otlptracehttp and otlptracegrpc "from scratch" (similar to otlploghttp).

More: #5221

Originally posted by @pellared in #2579 (comment)

@pellared pellared changed the title oteltracegrpc v2 otlptracegrpc v2 Apr 22, 2024
@pellared pellared added enhancement New feature or request area:trace Part of OpenTelemetry tracing pkg:exporter:otlp Related to the OTLP exporter package labels Apr 22, 2024
@pellared pellared moved this from Needs triage to Low priority in Go: Triage Apr 22, 2024
@github-project-automation github-project-automation bot moved this to Needs triage in Go: Triage Apr 22, 2024
@MrAlias
Copy link
Contributor

MrAlias commented Apr 22, 2024

We should only accept a grpc conn for the new package options instead of creating our own.

@pellared
Copy link
Member Author

We should only accept a grpc conn for the new package options instead of creating our own.

I think we should do the same for otlploggrpc, right?

Related issue: #5056
CC @XSAM

@dashpole
Copy link
Contributor

We should only accept a grpc conn for the new package options instead of creating our own.

Can we also keep DialOptions? It much more convenient, and allows passing many of the other options we support. I think we should keep the v1 of the otlploggrpc exporter consistent with the other signals, unless we plan to release a v2 of the others before we release the v1 of otlploggrpc.

@XSAM
Copy link
Member

XSAM commented Apr 29, 2024

We should only accept a grpc conn for the new package options instead of creating our own.

Can we also keep DialOptions? It much more convenient, and allows passing many of the other options we support. I think we should keep the v1 of the otlploggrpc exporter consistent with the other signals, unless we plan to release a v2 of the others before we release the v1 of otlploggrpc.

If we keep WithDialOption, we might also need to provide WithEndpoint, as the grpc.NewClient needs an endpoint unless we want to force our users to use env to provide the endpoint.

@XSAM
Copy link
Member

XSAM commented Apr 29, 2024

So, maybe we should just keep the WithGRPCConn.

@dashpole
Copy link
Contributor

I would support keeping WithEndpoint as well in that case.

@MrAlias
Copy link
Contributor

MrAlias commented May 2, 2024

We should only accept a grpc conn for the new package options instead of creating our own.

Can we also keep DialOptions? It much more convenient, and allows passing many of the other options we support. I think we should keep the v1 of the otlploggrpc exporter consistent with the other signals, unless we plan to release a v2 of the others before we release the v1 of otlploggrpc.

@dashpole can you provide some examples here? I'm not sure I follow how passing options via our WithDialOptions is easier than directly to the grpc.NewClient function.

I'm not opposed to keeping things the way they are in other modules, but I would want to better understand how our API is an improvement before copying it.

@MrAlias
Copy link
Contributor

MrAlias commented May 2, 2024

A use case the NewClient only option wouldn't support:

  • You want to pass client auth to be used
  • You also want to use environment variables or the defaults for endpoint, path, ...

The client only solution would mean you need to reproduce all the default/envar stuff on your own.

@pellared
Copy link
Member Author

With #5247 closed I do not think this issue makes sense.

@pellared pellared closed this as not planned Won't fix, can't repro, duplicate, stale May 10, 2024
@github-project-automation github-project-automation bot moved this from Low priority to Closed in Go: Triage May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:trace Part of OpenTelemetry tracing enhancement New feature or request pkg:exporter:otlp Related to the OTLP exporter package
Projects
Status: Closed
Development

No branches or pull requests

4 participants