Make Write message type more flexble, address some feedback #1710
Conversation
Signed-off-by: Saswata Mukherjee <[email protected]>
| // Under any marshaling scheme, v2 requests have a `Symbols` field of type []string. | ||
| // So would always have a `GetSymbols()` method which doesn't rely on any other types. | ||
| type v2Request interface { | ||
| GetSymbols() []string |
There was a problem hiding this comment.
This seems a bit hacky, but need it for the contentType detection. I wonder if there is a better way here
There was a problem hiding this comment.
Interesting. I think this is acceptable for now.
| // Under any marshaling scheme, v2 requests have a `Symbols` field of type []string. | ||
| // So would always have a `GetSymbols()` method which doesn't rely on any other types. | ||
| type v2Request interface { | ||
| GetSymbols() []string |
There was a problem hiding this comment.
Interesting. I think this is acceptable for now.
| @@ -74,6 +75,22 @@ func WithAPILogger(logger *slog.Logger) APIOption { | |||
| } | |||
| } | |||
|
|
|||
| // WithAPIEndpoint returns APIOption that allows providing endpoint. | |||
| func WithAPIEndpoint(endpoint string) APIOption { | |||
There was a problem hiding this comment.
You mean path? Using it in URL will mean path. Either we rename to path or not use client.URL or not use client structs at all.
I wanted to move this lib to separate client_golang/exp/ module for now, so we might want to get rid of client (improve it). Can do in my PR, but let's just switch to path here for now?
There was a problem hiding this comment.
Yeah it hit me after, let me update
There was a problem hiding this comment.
Switched to path, I can do exp module in next PR
| } | ||
|
|
||
| // NewRemoteWriteHandler returns HTTP handler that receives Remote Write 2.0 | ||
| // protocol https://prometheus.io/docs/specs/remote_write_spec_2_0/. | ||
| func NewRemoteWriteHandler(logger *slog.Logger, store writeStorage) http.Handler { | ||
| return &handler{logger: logger, store: store} | ||
| func NewRemoteWriteHandler(logger *slog.Logger, store writeStorage, decompressor remoteWriteDecompressor) http.Handler { |
There was a problem hiding this comment.
We might need opts here as well, separate ones 🤔
There was a problem hiding this comment.
Added for logger and decompressor
| @@ -279,18 +327,32 @@ type writeStorage interface { | |||
| Store(ctx context.Context, proto WriteProtoFullName, serializedRequest []byte) (_ WriteResponseStats, code int, _ error) | |||
| } | |||
|
|
|||
| // remoteWriteDecompressor is an interface that allows decompressing the body of the request. | |||
| type remoteWriteDecompressor interface { | |||
There was a problem hiding this comment.
can you remind me why we need this?
Another option is to use server middleware interface for this. There are common middlewares for this work ppl use e.g. Otel
There was a problem hiding this comment.
Some downstream users have their own request body size limiter+instrumentaion impls, that they would probably want to use. I know Thanos and Cortex have them #1658 (comment)
I don't think compression-wise things would be too different (maybe using s2).
There was a problem hiding this comment.
Cool, I will merge for now, but we need to switch to middleware injection technique
Signed-off-by: Saswata Mukherjee <[email protected]>
Addresses a few comments from PR
Also imports v1, as a convenience package as well (we can keep until we deprecate fully?)
Also makes Write marshaling more flexible with any type and gogo marshaling. Generics don't really seem to fit as you can't have multiple interface constraints on them.
Might be worth, merging with merge commit to keep original PR commits clear
cc: @bwplotka