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, exporterqueue] Deprecate unused public symbols #11285

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jade-guiton-dd
Copy link
Contributor

Description

To help stabilize the exporter module faster, this PR deprecates some public symbols from exporterhelper and the entirety of exporterqueue, which don't seem to be used by anyone on Github, so that they might be fully internalized in a future release.

(Another option would be removing them entirely, if some aren't used internally. Yet another would be moving them to a new module as suggested by #11143, if it turns out some are currently in use in the ecosystem.)

All the symbols involved were introduced by #8122; of the symbols introduced by that PR, only WithBatcher is used in the ecosystem, and thus isn't deprecated by this PR. It has a variable argument of type BatcherOption, which is deprecated as it isn't used. A future release should remove this variable argument.

To avoid the linter complaining about internal use of deprecated symbols, exporterqueue functionality was split into an internal package with most of the functionality, and deprecated aliases in the old exporterqueue package. The new internal package is exporter/internal/exporterqueue instead of exporter/exporterqueue/internal, to allow its internal use by exporter/exporterhelper.

An issue arose with the aliasing process: exporterqueue exports multiple generic types, but generic aliases are only supported starting from Go 1.23. So, unless we wish to update the Go version soon, the only option was to declare a different defined type, and convert at runtime between the two in the alias function shims. There is some surprising behavior in exactly which equivalent types Go allows to convert between, which leads to some strange code in said shims.

Link to tracking issue

Updates #11142

Copy link

codecov bot commented Sep 27, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 91.86%. Comparing base (5ac1607) to head (f8d9243).
Report is 87 commits behind head on main.

Files with missing lines Patch % Lines
exporter/exporterqueue/queue.go 0.00% 11 Missing ⚠️
exporter/exporterhelper/common.go 0.00% 3 Missing ⚠️
exporter/exporterqueue/config.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11285      +/-   ##
==========================================
- Coverage   91.91%   91.86%   -0.06%     
==========================================
  Files         432      434       +2     
  Lines       20350    20371      +21     
==========================================
+ Hits        18705    18713       +8     
- Misses       1271     1285      +14     
+ Partials      374      373       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jade-guiton-dd jade-guiton-dd force-pushed the exporterhelper-internalize-batching branch from adb2704 to 2c722a7 Compare September 30, 2024 11:47
@jade-guiton-dd jade-guiton-dd marked this pull request as ready for review October 1, 2024 11:36
@jade-guiton-dd jade-guiton-dd requested a review from a team as a code owner October 1, 2024 11:36
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one comment about deprecating the whole of exporterqueue

exporter/exporterqueue/config.go Show resolved Hide resolved
@bogdandrutu
Copy link
Member

I don't think we should "delete" everything that is not used. Please at least consider to wait until @dmitryax reviews and approves this.

@songy23 songy23 requested a review from dmitryax October 1, 2024 19:20
@jade-guiton-dd
Copy link
Contributor Author

@dmitryax do you have an opinion on how we should proceed regarding the batching API?

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added Stale and removed Stale labels Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants