bug: address parsing wavefront urls with a path component #109
Conversation
internal/reporter.go
Outdated
| return reporter.execute(req) | ||
| } | ||
|
|
||
| func linesToBuffer(pointLines string) (bytes.Buffer, error) { |
There was a problem hiding this comment.
I don't think bytes.Buffer supports copying by simple assignment. Instead of returning a bytes.Buffer or even a *bytes.Buffer consider instead returning a []byte. Returning a []byte is more concise and less confusing because you are eagerly constructing the bytes in this function anyway. Also consider renaming linesToBuffer to linesToBytes or something like that.
There was a problem hiding this comment.
I think what you're saying is that returning a bytes.Buffer is a form of copying by value and that the buffer might not end up containing the intended gzipped content. I haven't dug into that but I like the idea of returning the simpler []byte type - pushing that change now.
There was a problem hiding this comment.
Slightly concerned about whether the refactor, although more readable, might cause any noticeable performance changes given we're creating a new buffer twice now (I don't know if we think of this code as performance sensitive or have any performance tests around it)
There was a problem hiding this comment.
Regarding your concern about creating a buffer twice. The godocs for NewBuffer say: NewBuffer creates and initializes a new Buffer using buf as its initial contents. The new Buffer takes ownership of buf, and the caller should not use buf after this call. This makes me think that the new Buffer is using the existing slice for its storage which should be relatively cheap.
resolves: #108
This PR adds a "Path" field to the
configurationstruct so that we can stash the path component of a URL there and use it to correctly reconstruct a URL later.I would be interested in suggestions about the
MetricsURLandTracesURLmethods; I think exporting these is a little bit unfortunate, but the url construction is important to test and is currently a little hard to exercise in a test because it's happening inside unexported functions. We could refactor to make this easier to test, but I didn't want to increase the scope of the PR for this fix.This supports usecases where rather than talking to a WF instances or proxy directly, we are communicating through some sort of proxy that does path-based routing of our requests.
@keep94 @oppegard thanks in advance for your eyes on this PR 🙇♂️
cc: @rushikeshbgithub