Skip to content

loadbalancingexporter: add Start for exporters #1687

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

Merged

Conversation

pkositsyn
Copy link
Contributor

@pkositsyn pkositsyn commented Nov 23, 2020

Description:
Fixed a bug with not working loadbalancingexporter with retry queue. Also added sorting for endpoints on backend change (assume it is occasionally missing).

Link to tracking Issue: Fixes #1621

Testing:
Added test for endpoints sort

@pkositsyn pkositsyn requested a review from a team November 23, 2020 22:10
@pkositsyn
Copy link
Contributor Author

I don't know whether its ok to save host for later usage. @bogdandrutu Please verify that it's legal

@pkositsyn
Copy link
Contributor Author

Also I think this kind of bug is really painful. Can we prevent such things in future? Introduce unified end-to-end test for every new receiver and exporter or something.

@codecov
Copy link

codecov bot commented Nov 23, 2020

Codecov Report

Merging #1687 (3492288) into master (170cac0) will decrease coverage by 0.01%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1687      +/-   ##
==========================================
- Coverage   89.39%   89.37%   -0.02%     
==========================================
  Files         370      370              
  Lines       18115    18118       +3     
==========================================
  Hits        16193    16193              
- Misses       1432     1434       +2     
- Partials      490      491       +1     
Flag Coverage Δ
integration 70.97% <ø> (ø)
unit 88.06% <60.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
exporter/loadbalancingexporter/exporter.go 92.23% <60.00%> (-2.77%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 170cac0...3492288. Read the comment docs.

@bogdandrutu
Copy link
Member

@Vemmy124 we don't have any concern for the moment about not keeping a reference to the host. So it is ok.

@jpkrohling jpkrohling assigned jpkrohling and unassigned bogdandrutu Nov 24, 2020
@pkositsyn
Copy link
Contributor Author

@jpkrohling Resolved the issue in the latest commit

@bogdandrutu bogdandrutu merged commit efe52b1 into open-telemetry:master Nov 24, 2020
dyladan referenced this pull request in dynatrace-oss-contrib/opentelemetry-collector-contrib Jan 29, 2021
ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022
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.

loadbalancing: "sending_queue" causes OTLP exporter to not work
4 participants