Skip to content
This repository was archived by the owner on Jul 1, 2022. It is now read-only.

Conversation

@lenoch7
Copy link
Contributor

@lenoch7 lenoch7 commented May 3, 2021

Which problem is this PR solving?

  • repeatedly error logging when HttpSender is used
  • unfortunately does not solve the problem when UdpSender is used (see description)

Short description of the changes

Resolves #732 - repeatedly error logging when HttpSender is used. The problem was that removing "failed" command from commandFailedBefore HashSet was always done when command execution ends successfully (i.e. without throwing SenderException). But there are some situations, when "span is only saved in a buffer" on sender level (or there is no any span to be sent) and remote communication is not started at all.

Unfortunately the solution does not solve a problem if UdpSender is used. The reason is that PortUnreachableException is not thrown immediately, when DatagramSocket.send(DatagramPacket) method is invoked - this exception is thrown when subsequent call to send or receive is done. See Javadoc:

Fix problem with repeatedly error logging when HttpSender is used. The
problem was that removing "failed" command from 'commandFailedBefore'
HashSet was always done when command execution ends successfully (i.e.
without throwing SenderException). But there are some situations, when
"span is only saved in a buffer" on sender level (or there is no any
span to be sent) and remote communication is not started at all.

Unfortunatelly the solution does not solve a problem if UdpSender is
used. The reason is that PortUnreachableException is not thrown
immediately, when DatagramSocket.send(DatagramPacket) method is
invoked - this exception is thrown when subsequent call to send or
receive is done. See Javadoc:
 - DatagramSocket.send(DatagramPacket)
 - DatagramSocket.connect(InetAddress, int)

Signed-off-by: Radek Kraus <6397085+lenoch7@users.noreply.github.com>
@codecov
Copy link

codecov bot commented May 3, 2021

Codecov Report

Merging #785 (0f80c01) into master (7544006) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #785      +/-   ##
============================================
+ Coverage     89.03%   89.06%   +0.02%     
- Complexity      605      606       +1     
============================================
  Files            73       73              
  Lines          2261     2258       -3     
  Branches        295      295              
============================================
- Hits           2013     2011       -2     
  Misses          157      157              
+ Partials         91       90       -1     
Impacted Files Coverage Δ Complexity Δ
...egertracing/internal/reporters/RemoteReporter.java 90.32% <100.00%> (-0.31%) 7.00 <0.00> (ø)
...ing/internal/samplers/RemoteControlledSampler.java 92.50% <0.00%> (+1.25%) 17.00% <0.00%> (+1.00%)

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 7544006...0f80c01. Read the comment docs.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

lgtm, thanks

@yurishkuro yurishkuro merged commit 318dad2 into jaegertracing:master May 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RemoteReporter repeatedly logging error and success during unavailable agent

2 participants