fix: Improve request / response handling

[MikeGoldsmith]

Which problem is this PR solving?

We're seeing inconsistent request / response pairs being reported by the agent. After investigation, this came down to two problems:

1. Subsequent packets on the same channel would reuse the same key for storing & retrieval in the httpMatcher
2. Subsequent packets on the same channel used a single byte buffer, so if a second request was received before the httpMatcher finished reading the previous request, it would mix bytes from two packets together

Short description of the changes

• Add request counter to ensure each request / response gets a unique key for storing and retrieving entries
• Update httpMatcher to use a normal map and sync.Lock as we want to do lookup and store operations while being safe between these calls
• Introduce a message type and add a channel for each reader the stream can use to pass subsequent requests
• Update stream close behaviour to also close channels and byte buffers

How to verify that this has the expected result

Events should be more consistent in matching request & response pairs, meaning more events being dispatched.

[vreynolds]

Kicked the tires a bit on this, and so far looks good in my testing, but curious to see what others find. I'm also seeing non-negative durations!

[JamieDanielson]

This is great! I now have no events missing a request or response pair, and no negative durations 🎉

With Debug enabled, I do still see errors in the logs like malformed HTTP status code \"\\xbc\\xac0\\xf3\\x15\\xc1[mٱ\\xaf, but we can look further into that as a follow-up.

I've added a question on the swap out from sync.Map to regular go map, and a typo suggestion.

[JamieDanielson]

I admittedly have no experience using sync.Map outside of this, but I'm curious why we chose to swap it out for a regular Go map. Reviewing the docs, it says that while most code should use a plain Go map,

The Map type is optimized for two common use cases: (1) when the entry for a given key is only ever written once but read many times, as in caches that only grow, or (2) when multiple goroutines read, write, and overwrite entries for disjoint sets of keys. In these two cases, use of a Map may significantly reduce lock contention compared to a Go map paired with a separate Mutex or RWMutex.

I'm not sure if we fall into this category and would be curious to see results in dogfooding. I guess sync.Map may be premature optimization at the expense of type safety and common usage? This blog notes

The Go team identified situations in the standard lib where performance wasn’t great. There were cases where items were fetched from data structures wrapped in a sync.RWMutex, under high read scenarios while deployed on very high multi-core setups and performance suffered considerably.

The flip side is that using sync.Map for the wrong use case can result in lowered performance, so I'm not positive where our agent stands in these scenarios and curious for your take.

[MikeGoldsmith]

So we need to do a few operations:

• check if key exists
  • If yes;
    • update it with the request/response half we have
    • remove key from map
    • return entry
  • if no;
    • create new entry with request/response half we have
    • add to map
    • return nil

The checking if it exists and creating a new entry if not needs to be thread safe, as we could receive both the request and response at the same time and both do the look up to find no entry, create an entry and then race to set it in the map. The updated logic means we can do it all under one lock.

The sync.Map may be able to replace what we have but as you pointed out, in certain circumstances it's slower and I'm not sure it's clearer to use. I think the separate map and mutex is simple and easy to understand.