Set forwarded info as separate tag rather than replace peer #2677
Conversation
tylerbenson
force-pushed
the
tyler/forwarded-tag
branch
from
c90daaa
to
1adfe91
Compare
...rap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/HttpServerDecorator.java
Outdated
Show resolved
Hide resolved
| 1 * ctx.getForwardedFor() >> (ipv4 ? "10.1.1.1" : "0::1") | ||
| 1 * ctx.getForwardedPort() >> "123" | ||
| 2 * ctx.getForwarded() >> "by=<identifier>;for=<identifier>;host=<host>;proto=<http|https>" | ||
| 2 * ctx.getForwardedProto() >> "https" | ||
| 2 * ctx.getForwardedHost() >> "somehost" | ||
| 2 * ctx.getForwardedIp() >> (ipv4 ? "10.1.1.1, 192.168.1.1" : "0::1") | ||
| 2 * ctx.getForwardedPort() >> "123" | ||
| 1 * span.setTag(Tags.HTTP_FORWARDED, "by=<identifier>;for=<identifier>;host=<host>;proto=<http|https>") | ||
| 1 * span.setTag(Tags.HTTP_FORWARDED_PROTO, "https") | ||
| 1 * span.setTag(Tags.HTTP_FORWARDED_HOST, "somehost") | ||
| if (ipv4) { | ||
| 1 * span.setTag(Tags.PEER_HOST_IPV4, "10.1.1.1") | ||
| 1 * span.setTag(Tags.HTTP_FORWARDED_IP, "10.1.1.1, 192.168.1.1") | ||
| 1 * span.setTag(Tags.PEER_HOST_IPV4, "10.0.0.1") | ||
| } else if (conn?.ip) { | ||
| 1 * span.setTag(Tags.HTTP_FORWARDED_IP, "0::1") | ||
| 1 * span.setTag(Tags.PEER_HOST_IPV6, "3ffe:1900:4545:3:200:f8ff:fe21:67cf") | ||
| } else { | ||
| 1 * span.setTag(Tags.PEER_HOST_IPV6, "0::1") | ||
| 1 * span.setTag(Tags.HTTP_FORWARDED_IP, "0::1") | ||
| } | ||
| 1 * span.setTag(Tags.HTTP_FORWARDED_PORT, "123") | ||
| if (conn) { | ||
| 1 * span.setTag(Tags.PEER_PORT, 555) | ||
| } | ||
| 1 * span.setTag(Tags.PEER_PORT, "123") |
There was a problem hiding this comment.
It's not clear this tests anything except for the way you wrote the code. Please attempt to satisfy the request here without changing this test. If you find that you can't, your test isn't testing what the code does but how it does it, which is extremely hostile even to trivial refactoring.
There was a problem hiding this comment.
I changed the mocks to not care about the number of invocations. I think that should resolve your concern.
There was a problem hiding this comment.
Mocks feel like overkill for this. Although - admittedly, I'm not really a fan of mocking libraries period.
Why don't we just check the tags via a getTag mechanism? Tracking calls to setTag just seems odd to me.
There was a problem hiding this comment.
I just don't like tests like these.
- They make it hard to refactor things because they end up specifying the implementation, normalising refactoring leading to destructive - as opposed to purely additive - changes to tests.
- Sometimes they inadvertently verify interactions not required of the system under test in order to provoke behaviour, so a refactor can occasionally require a full run of the tests to detect all test assumptions violated (Groovy does not help here, where compiler errors would be a boon).
- It's not always clear what is being tested and what is being provoked, and given that virtually any refactoring will lead to modification of the test, this risks refactoring leading to loss of specification.
- Worst of all, I have found one case in our test suite where the mocks tested impossible scenarios by ignoring the semantics of the system under test - in this parallel universe concocted by mocks, a class's superclass could have been an interface, so a refactor to take advantage of the fact that a class's superclass is not an interface failed.
dd-trace-core/src/main/java/datadog/trace/core/propagation/ContextInterpreter.java
Outdated
Show resolved
Hide resolved
tylerbenson
force-pushed
the
tyler/forwarded-tag
branch
3 times, most recently
from
d30e9de
to
0512a85
Compare
| "$Tags.PEER_PORT" Integer | ||
| "$Tags.HTTP_URL" "${endpoint.resolve(address)}" | ||
| "$Tags.HTTP_METHOD" method | ||
| "$Tags.HTTP_STATUS" endpoint.status | ||
| if (endpoint == FORWARDED) { |
There was a problem hiding this comment.
Why do we have so much repeated code here?
I though the whole point of the HttpServerTest hierarchy was to avoid duplication.
There was a problem hiding this comment.
This was the hacky way to handle a framework that reported traces which didn't match the base validation method. This is one of the biggest areas I want to improve when we refactor the assertion framework.
| } else { | ||
| span.setTag(Tags.PEER_HOST_IPV4, ip); | ||
| if (connection != null) { | ||
| final String ip = peerHostIP(connection); |
There was a problem hiding this comment.
This was previously set to the x-forwarded-for if present, and now it always picks up the peerHostIP from the connection. I just feel that I'm missing some explanation as to why this change in behavior is ok/desirable.
There was a problem hiding this comment.
As hinted in the description, there is some subtle differences in how some frameworks handle comma separated lists of values in the header. For example, RemoteIpValve is configurable and often filters out local (eg 10.x, 192.168.x) ip addresses. Rather than match that flexibility, I think it's better to just present all the information.
This avoids the complexity of managing multiple levels of forwarding presented as a comma separated list. Also captures additional headers: `forwarded`, `x-forwarded-proto`, `x-forwarded-host`
tylerbenson
force-pushed
the
tyler/forwarded-tag
branch
from
0512a85
to
178a5ec
Compare
This avoids the complexity of managing multiple levels of forwarding presented as a comma separated list.
Also captures additional headers:
forwarded,x-forwarded-proto,x-forwarded-host