-
Notifications
You must be signed in to change notification settings - Fork 9.6k
write_handler: Use client_golang/exp/api/remote for handling remote writes #16086
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. We removed more than add, so our helper helped (:
LGTM 💪🏽
@@ -376,7 +377,7 @@ func main() { | |||
a.Flag("web.enable-remote-write-receiver", "Enable API endpoint accepting remote write requests."). | |||
Default("false").BoolVar(&cfg.web.EnableRemoteWriteReceiver) | |||
|
|||
supportedRemoteWriteProtoMsgs := config.RemoteWriteProtoMsgs{config.RemoteWriteProtoMsgV1, config.RemoteWriteProtoMsgV2} | |||
supportedRemoteWriteProtoMsgs := remoteapi.MessageTypes{remoteapi.WriteV1MessageType, remoteapi.WriteV2MessageType} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could replace config entries as well with remoteapi perhaps? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so I did most of the constants, but some still remain. I'll modify that in next PR for replacing client/sending code.
@@ -52,6 +52,7 @@ require ( | |||
github.com/ovh/go-ovh v1.6.0 | |||
github.com/prometheus/alertmanager v0.28.0 | |||
github.com/prometheus/client_golang v1.21.0-rc.0 | |||
github.com/prometheus/client_golang/exp v0.0.0-20250227122456-ad23ad6d5468 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wanna go ahead and make v0.1.0 release on client_golang? Likely the tag has to be in form of exp v0.1.0
(https://stackoverflow.com/a/77864755) if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup totally. I think https://github.com/hashicorp/vault/tags does a similar thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's hold off tagging, until I have PR for replacing some of the client code, that way, we can release v0.1.0 with exp we know works.
…rites Signed-off-by: Saswata Mukherjee <[email protected]>
Signed-off-by: Saswata Mukherjee <[email protected]>
Signed-off-by: Saswata Mukherjee <[email protected]>
Signed-off-by: Saswata Mukherjee <[email protected]>
Signed-off-by: Saswata Mukherjee <[email protected]>
Why would we rely on something "experimental" to implement something declared stable? |
@bboreham do you mean remote write protocol 1.0 stability? That's fair, the ecosystem relies on it. However the stability is for API, not for quality of the code. For this work we want to ensure the wider ecosystem can use the same stable behaviours for both remote write 1.0, 2.0 and beyond. In order to do so we extracted relevant pieces to an external API which is now experimental as we may shape the API a bit (and since we are in the same team, making sure it's working for Prometheus well). This means only more stability for projects like Mimir, Thanos, Cortex and senders like Alloy, Tempo, avalanche, etc To sum up, we don't change any protocol behaviours here AFAIK (we carefully checked that). If you are worried about the risk, we don't risk anything more than when we were (and will be) adjusting both the handler and writer for 2.0 and future work here. WDYT? |
This PR uses the new client_golang experimental remote API package, introduced in prometheus/client_golang#1658 to handle remote writes (v1 and v2).
Also, removes a couple of test cases around missing headers/no content-type as those are now handled by the exp write handler (assumes V1 and snappy as defaults).
cc: @bwplotka