[Heartbeat] Fix missing url.* fields on TCP NXDOMAIN #10787
Conversation
andrewvc
added
bug
review
needs_backport
|
Pinging @elastic/uptime |
andrewvc
force-pushed
the
nxdomain-fix-7
branch
from
51ad00a
to
8d33ef2
Compare
| @@ -91,10 +91,6 @@ func setupServer(t *testing.T, serverCreator func(http.Handler) *httptest.Server | |||
| return server, port | |||
| } | |||
|
|
|||
| func tcpMonitorChecks(host string, ip string, port uint16, status string) mapval.Validator { | |||
There was a problem hiding this comment.
This helper function had a number of unused arguments and was more confusing than helpful.
There was a problem hiding this comment.
Overall LGTM, perhaps you can help me out in one comment below. This needs a changelog entry.
A more general comment. One thing I would like to see is tests that generate golden files which can be compared like I introduced here for Metricbeat: #10648 For me this would give me more confidence reviewing such PR's. We have the mapval tests which do a very similar thing but having the exact end json and seeing it's modified or not would be pretty useful. Also I would hope this would make it very easy to add new testing scenarios by specifying the endpoint it should ping and then generate the expected output. The data.json which would be generated could also be used in the documentation as examples.
| return err | ||
| } | ||
|
|
||
| eventext.MergeEventFields(event, common.MapStr{"url": wrappers.URLFields(u)}) |
There was a problem hiding this comment.
Trying to find out where this is now ...
There was a problem hiding this comment.
That's been moved here: https://github.com/elastic/beats/pull/10787/files/8d33ef28de0ef07f1e8d26318aba21f7c20bff89#diff-ff8455f217780e07b3802951d4451126R151
I'll open a future refactor after this is merged to move dialchain/builder.go into the tcp package. Everything in that file is actually only used by TCP, and it's confusing that there's TCP specific code that looks very generic in dialchain/builder.go.
|
@ruflin I don't think it needs a changelog because this bug was never released. Golden files would not have caught this because it only applies to ICMP. Mapval tests would have caught it just as well, but the reason we don't have them is due to the difficulty of testing ICMP. Indeed there are mapval tests for HTTP and TCP that would have caught this bug there should it have existed. Additionally, golden files don't work well for heartbeat because a lot of the data is timing data which is variable, so there's no golden number. Mapval works well for describing the shape of data. Going forward it's clear that we need to find a way to run ICMP tests. Options there are:
In general I favor the former as it means we'd be testing the real codepath. |
|
@ruflin BTW, ready for a re-review, I think I've addressed all issues here. |
TCP checks are not adding URL fields on NXDOMAIN endpoints. This fixes that issue.
It does so by ensuring that URL metadata is added before executing the check, and not during, as done previously.
A side effect of this is that we now perform DNS lookups once per `{hostname,port}` instead of once per `{hostname}`. This is worth the increased simplicity however, as the code would be quite convoluted otherwise, which would put us at risk for more bugs.
Related (but different) 6.x issue: elastic#10777
Fix import formatting
andrewvc
force-pushed
the
nxdomain-fix-7
branch
from
9f0ac9f
to
eb6ec2b
Compare
|
Merging after resolving a conflict in some tests. |
TCP checks are not adding URL fields on NXDOMAIN endpoints. This fixes that issue.
It does so by ensuring that URL metadata is added before executing the check, and not during, as done previously.
A side effect of this is that we now perform DNS lookups once per `{hostname,port}` instead of once per `{hostname}`. This is worth the increased simplicity however, as the code would be quite convoluted otherwise, which would put us at risk for more bugs.
Related (but different) 6.x issue: elastic#10777
Fix import formatting
(cherry picked from commit 821baec)
andrewvc
added
v7.0.0
and removed
needs_backport
TCP checks are not adding URL fields on NXDOMAIN endpoints. This fixes that issue.
It does so by ensuring that URL metadata is added before executing the check, and not during, as done previously.
A side effect of this is that we now perform DNS lookups once per `{hostname,port}` instead of once per `{hostname}`. This is worth the increased simplicity however, as the code would be quite convoluted otherwise, which would put us at risk for more bugs.
Related (but different) 6.x issue: #10777
Fix import formatting
(cherry picked from commit 821baec)
TCP checks are not adding URL fields on NXDOMAIN endpoints. This fixes that issue.
It does so by ensuring that URL metadata is added before executing the check, and not during, as done previously.
A side effect of this is that we now perform DNS lookups once per
{hostname,port}instead of once per{hostname}. This is worth the increased simplicity however, as the code would be quite convoluted otherwise, which would put us at risk for more bugs.Related (but different) 6.x issue: #10777