Refactor: Unify metadata and samples sends to ingesters #2410
Conversation
Previously, we had samples and metadata using the ring on their own to send data to the ingesters. Since the `WriteRequest` supports metadata alongside samples there's no reason to split the calls so we unify them. There's an additional tiny refactor to inline the metadata validation as it is just a single function call. Signed-off-by: gotjosh <[email protected]>
gotjosh
force-pushed
the
unify-metadata-samples
branch
from
53c6865 to
d48e918
Compare
| } | ||
|
|
||
| if len(req.Metadata) > 0 { | ||
| // Given requests can only contain either metadata or samples, no-op if there is metadata for now. |
There was a problem hiding this comment.
Technically, this is "no longer true" from an implementation perspective. The remote write protocol only sends metadata or samples (for now) but IMO that comment/responsibility belongs in the distributor. The ingester should operate under no assumption that you'd receive one or the other if we can (e.g. no increase of complexity).
Obviously, this is my own personal opinion with what I know about the system, but others feel free to suggest any alternatives.
There was a problem hiding this comment.
If we're going to handle samples and metadata in the same push, then I would suggest that we return errors for them separately. As it is now, errors reported by ingesters will be counted for both samples and metadata (provided both were present in the request).
There was a problem hiding this comment.
Sorry Peter, I'm not following your comment. With this change, we're no longer making assumptions that a WriteRequest can come with one or the other.
The error handling in send is purely for metrics reporting. In the ingesters, we simply log when we receive metadata, so erroring out on that is not an option at the moment.
The comment I originally left in the distributors is an operational concern, but for the code itself, it wouldn't make a difference - that is indented.
|
Should I be updating any tests? I didn't see anything that felt worth updating but feel free to suggest otherwise. |
| } | ||
|
|
||
| if len(req.Metadata) > 0 { | ||
| // Given requests can only contain either metadata or samples, no-op if there is metadata for now. |
There was a problem hiding this comment.
If we're going to handle samples and metadata in the same push, then I would suggest that we return errors for them separately. As it is now, errors reported by ingesters will be counted for both samples and metadata (provided both were present in the request).
Signed-off-by: gotjosh <[email protected]>
3 participants
What this PR does:
Previously, we had samples and metadata using the ring on their own to
send data to the ingesters. Since the
WriteRequestsupports metadataalongside samples there's no reason to split the calls so we unify them. There's an
additional tiny refactor to inline the metadata validation as it is just
a single function call.
Signed-off-by: gotjosh [email protected]
Which issue(s) this PR fixes:
A follow up from #2336
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]