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

[exporterhelper] Review usage of API and internalize anything not currently used #11142

Closed
mx-psi opened this issue Sep 11, 2024 · 3 comments
Closed
Assignees
Labels

Comments

@mx-psi
Copy link
Member

mx-psi commented Sep 11, 2024

There are parts of exporterhelper, exporterqueue that are not currently used by any component. We can make these parts internal for now to reduce the amount of API that we stabilize with exporter 1.0. One such example is exporterqueue.Config.

@mx-psi
Copy link
Member Author

mx-psi commented Sep 19, 2024

As an example here is a Github search that shows that exporterhelper.RequestErrorHandler is not used anywhere: https://github.com/search?q=language%3AGo+-is%3Afork+%2Fexporterhelper%5C.RequestErrorHandler%2F&type=code

To complete this issue we need to:

  1. Search on Github for the symbols to see if they are used or not
  2. If they are not used, open a first PR to deprecate the symbol(s)
  3. After the symbol(s) have been deprecated for one release, remove them

@mx-psi mx-psi added the good first issue Good for newcomers label Sep 19, 2024
@jade-guiton-dd
Copy link
Contributor

jade-guiton-dd commented Sep 20, 2024

After searching for all uses of public symbols from exporterhelper and exporterqueue outside of themselves on Github (including some cases where the package was renamed on import), it seems that the unused symbols are those introduced by #8122 and marked as "experimental".

This concerns all of exporterqueue, and the following from exporterhelper:

  • Functions New(Logs|Metrics|Traces)RequestExporter, WithRequestBatchFuncs, WithRequestQueue
  • Types BatcherOption, Request, RequestErrorHandler, RequestFrom(Logs|Metrics|Traces)Func

The only experimental symbol which is used is exporterhelper.WithBatcher. Note that it takes a vararg argument of type BatcherOption, but I found no use of it.

Edit: This issue conflicts with #11143, which suggests moving these features to a new package instead of removing them. After discussing it with @dmitryax who implemented most of this, making the symbols internal is probably the easiest option. If it turns out someone uses them, then we can move them and make them public again.

@jade-guiton-dd
Copy link
Contributor

No clear decision has been taken for what to do with the batching API, closing this for now.

@jade-guiton-dd jade-guiton-dd closed this as not planned Won't fix, can't repro, duplicate, stale Jan 7, 2025
@github-project-automation github-project-automation bot moved this from Blocked to Done in Collector: v1 Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

No branches or pull requests

2 participants