Add Moxy tool to apm-perf #158
Conversation
|
A general question for new code, specially smaller ones like this, wouldn't it be better to use the more minimal: https://pkg.go.dev/log/slog package? |
That might work (not sure if the API is similar to zap) although the proposition here may be that a consistent logger across services makes it easier to parse/aggregate in the external components collecting and storing the data. |
| payload io.Reader | ||
| headers http.Header | ||
| options []StubESOption | ||
| expectedCode int |
There was a problem hiding this comment.
I would rename this to expectedStatusCode
| options := []proxy.StubESOption{ | ||
| proxy.StubESWithLogger(logger), | ||
| } | ||
| if *username != "" && *password != "" { |
There was a problem hiding this comment.
Since this is a mock. I'd set username and password to a default. Then maybe add an explicit check to ensure both are set. It makes no sense to continue of either are empty. Unless, you want to have auth optional which seems to be the case here. But this weird to me, since it is a mock:
if *username == "" {
// fatal error
}
if *password == "" {
// fatal error
}
options = append(options, proxy.StubESWithAuth(*username, *password))If you want auth to be optional, maybe add a comment why? Maybe for performance testing?
There was a problem hiding this comment.
Auth there is just for an extra check that nobody sends unsolicited traffic when it's exposed publicly, otherwise it's optional.
| _, _ = w.Write(h.license) | ||
| return | ||
| case "/_bulk": | ||
| first := true |
There was a problem hiding this comment.
Move first := true to line 123.
| }` | ||
| ) | ||
|
|
||
| var memPool = sync.Pool{ |
There was a problem hiding this comment.
Interesting use of sync pool with buffering. Maybe add a comment or a link to this trick?
There was a problem hiding this comment.
I'm of the opinion to use []byte rather than bytes.Buffer. Either use sync.Pool or bytes.Buffer. Don't merge them:
sync.Poolis already an abstraction. Use the lowest-level "primitive" which is[]byte, rather than dealing with a higher-level abstraction likebytes.Buffer.- With
[]byteyou can simple reset the slice length.bytes.Bufferhas more internal state to manage. bytes.Bufferis more efficient to stream data, like appending, since it avoids frequent reallocations. However, in pooling you'd rather want a static "container" that move in-and-out of the pool.- You don't have need to use
bytes.BufferAPI and all the abstraction overhead that comes with it.
There was a problem hiding this comment.
As discussed separately bytes.Buffer and sync.Pool can compliment each other when reducing GC footprint for frequent allocations where the intermediate memory buffer size is unknown.
This PR is a part of the work on the issue.
To introduce a new benchmarks mode in APM Server we need a tool that simulates ElasticSearch API (Moxy).