Feature/http sink

[colmsnowplow]

Draft PR for http sink.

Notes

• It's not perfect but it works - likely a lot of improvements could be made.
• Current implementation omits qs for post, omits body for get, and sends a hardcoded get for HEAD
• We still generate data for all formats every time, which is inefficient. I can see how to refactor it to improve this but don't have the time - it's already in an issue.
• I think it'd be beneficial to be able to configure paths/weights of the different path options - I hope to get to adding this separately for this release, but we don't need it for collector so I'm first going to move to the deployment of this asset as an RC.
• There is a bug whereby the data generated itself is producing corrupt context data sometimes - which when base64-encoded cx field results in a JSON with the character '7' at the end (When decoded). It results in enrichment failures - so not critical to current use case but I'll log an issue.

TODO this release:

• Address review feedback
• Make distribution of GET/POST/HEAD configurable
• Fix origin headers
• Fix HEAD requests (or do something sensible with them) - Won't do - the server gives an error. Just leaving unimplemented for now.

To make tickets for:

• Bug whereby cx field in querystring produces a 7 character at the end of the b64 when decoded: Querystring data is often corrupt #60
• Make paths distribution configurable - this interacts with request type so needs some config design consideration: Make http paths configurable #61
• Make body/querystring generation configurable - sometimes we want a specific combination of both so needs some config design consideration: Http: make body/querystring combination configurable #62
• Refactor things function so that we run only the Gen functions we need rather than running all of them: Run only the Gen functions we need #63
• Refactor things such that the same Body data is used for all 3 gen functions - so that for the same seed, different formats of the same data are produced: Use the same data for any sink #64

[istreeter]

Unusual to rename from Upper case name to lower case name. More normal to write {Method => TrackerMethod}

[istreeter]

A few comments about this little section:

Easy one -- consider using IllegalArgumentException instead of Exception.

Another easy one -- It's unusual to use the .format syntax in scala. It's more normal to write it like

s"blah blah ${properties.endpoint} blah blah ${generatedRequest.method.path}"

A nice change you could make is to move the Uri.fromString(...) outside of this buildRequest block. So you only run it once when the app first starts up. And you only need to handle the error scenario once.

Then, once you have generated a Uri, you can use syntax like:

val address = baseUri / generatedRequest.method.path.vendor / generatedRequest.method.path.version

[istreeter]

Here when you call httpClient.use { ..... } it creates and destroys the http client + all of the internal objects needed to implement it. Like a TCP connection pool etc.

The problem is, you call .use for every single request. So you're continually creating/destroying the resources, which is quite wasteful.

It's better to find a way to .use the http client just once, and then re-use it for every request. I'd be happy to show you a few ways to arrange the code for this.

[istreeter]

I agree this is the right approach. I think we can make it neater though!

[istreeter]

If you made endpoint a org.http4s.Uri instead of a String in the config, then you wouldn't need to handle parsing exceptions later on when generating requests.

[colmsnowplow]

Note: HEAD requests are still not implemented. Using client.status instead of client.expect does prevent the app from crashing with these requests, but they all get:

This hits org.http4s.client.UnexpectedStatus: unexpected HTTP status: 422 Unprocessable Entity for request HEAD

Since I suspect that this is something to do with the request itself, rather than an expected error from the collector, I've left it for another day.

I have also made a separate PR to make request paths configurable. With the current implementation, we can generate /r/* paths, for which the collector correctly responds with 4XX or 3XX responses. Since it's desirable that for some of our tests (eg load stress testing) expect 100% of the data to succeed (ie. where failure responses are indicators of the limits of the infrastructure), and it was fairly simple to implement (while other work was blocked), I made the PR.

[colmsnowplow]

@istreeter it doesn't need your urgent attention, I've also flagged it to @peel who will likely soon be using the tool - but as an FYI/because I think you might be interested, see last comment for where things lie.

[colmsnowplow]

Note: Formatting might be off. I realised that I hadn't run scalafmt long after I had separately branched off. I plan to do that before release, just holding off until after I have to rebase one or more of these branches. :)

[peel]

The idea of being able to tune HTTP request formats is fantastic and overall this feature is massively helpful. Great job!

Please do run formatting and remove dead code. A few extra (hopefully helpful) comments below.

[peel]

If there's one thing that I'd change is the org.http4s.Uri instead of String in the config. This way you don't need to additionally validate the config here. Therefore sink becomes less defensive behavioural function. But the config is not affected, so we can do that at a better time.

[colmsnowplow]

Sorry Peel I'm not sure I understand what you're saying here?

[peel]

This is nice example where you'd be able to use a massively helpful quickcheck/property-based testing approach that's often used within Scala/FP-projects. We shouldn't do any of that now, but sharing for the sake of it.

The approach here would likely be:

• generate a collection of header name and it's values making sure Origin appears there at least once
• collect them into a desired format, where each header's values are comma-separated - that will be the headers argument value
• expected return value is the original collection where each key's value is a single Raw
• run through parseHeaders and compare input and output

The benefit of this approach is we cover all the scenarios and hint that we don't really care about the values but the shape of it. Next time we get around to looking at the kind of logic we'd know what the author cared about and the comment itself becomes a spec.

[colmsnowplow]

Yeah so I didn't touch the data generation part, but I did feel that tweaks to that implementation could make our lives easier here/make our approach more idiomatic - which I think is what you're getting at here, if I understand correctly.

[peel]

Again, not a reason for changing anything. But maybe that's a personal preference, but I've always found a function should have a single reason to change. Never been a massive fan of internal methods especially as it makes them hard to test individually.

I'd structure this function in a way that address, body, parseHeaders are separate functions, this way buildRequest could potentially look like this:

def buildRequesst(generatedRequest: HttpRequest): Request[F] = generatedRequest.method match {
  case TrackerMethod.Post(_) =>  Request[F](...)
  ...
}

def address(_) = _
def body(_) = _ 
def parseHeaders(_) = _

[colmsnowplow]

So my approach was to implement something that works, then look at how to refactor. I don't think I disagree with your perspective but at the same time, as things stood I didn't see an obvious way that a refactor would simplify/improve things - at least from the point of view that actually we don't seem to be repeating anything necessarily.

Not that I think it can't be factored better/more nicely - I just didn't see a quick win and didn't want to let perfect be the enemy of good. If support rota is quiet for the rest of the week I might get the chance to revisit this. :)

[colmsnowplow]

@peel / @istreeter I think it's ready for final review now, removed dead code and now parsing endpoint as Uri in config.

I will rebase commits before merging just didn't wanna make the convos hard to track. :)

Edit: I have just realised it's already got approval. I'll rebase and merge, apologies for unnecessary noise.