[exporter/loadbalancing] Fix inconsistent handling of endpoints without port #13745
Conversation
…ever contain host-strings with port
| // verify | ||
| assert.Len(t, p.exporters, 1) | ||
| assert.NotContains(t, p.exporters, "endpoint-2") | ||
| assert.Contains(t, p.exporters, endpointWithPort("endpoint-1")) |
There was a problem hiding this comment.
While the effect is the same, we should indeed assert that endpoint-2 does not exist here, as it has been removed.
There was a problem hiding this comment.
Sure. I figured asserting that the Len is 1, and that the 1 is endpoint-1 should cover all cases. But I can revert that.
| // verify | ||
| assert.Len(t, p.exporters, 1) | ||
| assert.NotContains(t, p.exporters, "endpoint-2") | ||
| assert.Contains(t, p.exporters, "endpoint-1:4317") |
There was a problem hiding this comment.
This one was a good change, thanks for that!
| assert.Nil(t, res) | ||
| } | ||
|
|
||
| func TestConsumeLogsExporterNotFound(t *testing.T) { |
There was a problem hiding this comment.
It no longer works. It seemed to rely on the bug I fixed, i.e. that removeMissing would remove some endpoints. There's no way to make
res := p.ConsumeLogs(context.Background(), simpleLogs())
fail in this way.
| } | ||
| } | ||
|
|
||
| func TestConsumeTracesExporterNotFound(t *testing.T) { |
There was a problem hiding this comment.
Ditto. It relied on the broken behavior to 'work'.
No, I didn't consider that, mainly because I think that could cause issues if we wind up with an endpoint with the same hostname, but different ports, like Edit: It's not possible in the dns resolver, but with the static resolver? Any future resolver? |
True, it's indeed realistic to have that as part of the static resolver. I think we even use that in the examples. |
|
Fixes #10321 |
Description:
Fixes the problem where a dns-resolver is defined without a
Portelement. This SHOULD result in the default port (4317) being used, but leads to some problems. See linked issue.The gist of the fix is to ensure that the strings in the lb.exporters map always have the port, and are always looked up with the port. Some of the unit-tests (logs and traces) can no longer happen as written and have been removed.
A deeper fix would likely require more refactoring, since the host (with or without port) is also used in the
itemsring.Link to tracking Issue:
#10321
Testing:
Unit tests performed.
Ran test-config from the linked issue, which worked.
Documentation:
No documentation added or changed.