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

Rename exporter-collector #2364

Closed
CptSchnitz opened this issue Jul 19, 2021 · 14 comments · Fixed by #2476
Closed

Rename exporter-collector #2364

CptSchnitz opened this issue Jul 19, 2021 · 14 comments · Fixed by #2476
Assignees
Labels
Discussion Issue or PR that needs/is extended discussion.

Comments

@CptSchnitz
Copy link
Contributor

It would be a good idea to rename the exporter to a better and clearer name.

A name like exporter-otlp will be easier to understand for newer people, and inline with the rest of the SDKs.

Also before GA is a good time to make such a change.

@vmarchaud vmarchaud added the Discussion Issue or PR that needs/is extended discussion. label Jul 19, 2021
@vmarchaud
Copy link
Member

cc @open-telemetry/javascript-approvers

@wong2
Copy link

wong2 commented Sep 6, 2021

yes, it's confusing

@rauno56
Copy link
Member

rauno56 commented Sep 7, 2021

@CptSchnitz, this seems to have quite a lot of support, would you like to draft a PR for it?

@CptSchnitz
Copy link
Contributor Author

sure!

@CptSchnitz
Copy link
Contributor Author

@rauno56 I got some questions

  1. should it be renamed to exporter-otlp or exporter-otlp-http?
  2. should the other exporter collector be renamed (proto and grpc)?

@wong2
Copy link

wong2 commented Sep 8, 2021

+1 for exporter-otlp-http

It takes me a while to realize it's a http exporter instead of some base exporter others relying

@vreynolds
Copy link
Contributor

If I understand it correctly, there's more nuance:

  • opentelemetry-exporter-collector is OTLP/HTTP (JSON)
  • opentelemetry-exporter-proto is OTLP/HTTP (binary)
  • opentelemetry-exporter-grpc is OTLP/gRPC

exporter-otlp-http-json? 😅

@rauno56
Copy link
Member

rauno56 commented Sep 9, 2021

If I understand it correctly, there's more nuance:

Had to look it up, but that's my understanding as well.

I'm sure there will be different preferences for that, and "the absolute most correctest" way would probably be to mention both the transport and the format as @vreynolds put it.

I'd personally leave some of that information implied.

  • exporter-otlp-json - implied http
  • exporter-otlp-http - implied proto
  • exporter-otlp-grpc

... but that's just me! :)

@CptSchnitz
Copy link
Contributor Author

hi didn't had the time to work on it yet and just started.
I got another question:
should the rename also include all the internal code mentions of the collector in the packages, or just the exported classes and interfaces? it makes the PR much bigger, and it can be done in a later stage, or separately for each package.

@vmarchaud
Copy link
Member

should the rename also include all the internal code mentions of the collector in the packages, or just the exported classes and interfaces

Only the exported classes make more sense, we could improve it later on

@obecny
Copy link
Member

obecny commented Sep 15, 2021

my 3 cents:

  • exporter-otlp-json
  • exporter-otlp-proto
  • exporter-otlp-grpc

@CptSchnitz
Copy link
Contributor Author

my 3 cents:

  • exporter-otlp-json
  • exporter-otlp-proto
  • exporter-otlp-grpc

i agree, the only thing that will be slightly "weird" is that both proto and grpc will depend on the json because it has the base class.

@vmarchaud
Copy link
Member

i agree, the only thing that will be slightly "weird" is that both proto and grpc will depend on the json because it has the base class.

Thats just implementation details, we could in the future make a dedicated util pkg that host base classes and remove the direct dependency so i think thats fine to do it like this

@dyladan
Copy link
Member

dyladan commented Sep 15, 2021

#2476

@dyladan dyladan linked a pull request Sep 15, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Issue or PR that needs/is extended discussion.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants