Skip to content

exp: Add README, address feedback, use sync.Pool #1747

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Feb 25, 2025
Merged

exp: Add README, address feedback, use sync.Pool #1747

merged 5 commits into from
Feb 25, 2025

Conversation

saswatamcode
Copy link
Member

@saswatamcode saswatamcode commented Feb 24, 2025 •
edited
Loading

This PR,

  • Adds a README for the exp module
  • Adds an APIOption for customizing http.Client to use (or use http.DefaultClient)
  • Modifies all proto header constants to be *ContentType
  • Modifies handler Store interface to use http.Request, and return new WriteResponse struct (stats + code + extra response headers)
  • Use sync.Pool for buffers everywhere (also to make API thread safe)

@saswatamcode
Use sync.Pool
49fa6d1 
Signed-off-by: Saswata Mukherjee <[email protected]>
bwplotka
bwplotka approved these changes Feb 25, 2025
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just small nits and ideas for future! Amazing!

exp/api/remote/remote_headers.go
@@ -94,6 +94,26 @@ const (
writtenExemplarsHeader = "X-Prometheus-Remote-Write-Exemplars-Written"
)

// WriteResponse represents the response from the remote storage upon receiving a remote write request.
type WriteResponse struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's room to make this easy to CREATE by creating helpers, methods and removing ability to unnecessary read it.

But we can just place comment "TODO" and do it later, ok to merge without it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok embedded response stats into the WriteResponse, made all fields private and added some create/setter/getter, and modified Add a bit to make the entire thing a bit cleaner.

Should be easy to construct this now :)

@bwplotka bwplotka merged commit 325580d into prometheus:prwclient-em Feb 25, 2025
7 checks passed
bwplotka added a commit that referenced this pull request Feb 25, 2025
@bwplotka @saswatamcode
exp/api: Add experimental exp module; Add remote API with write clien…
248c3f7 
…t and handler. (#1658)

* api: Add remote API with write client; add remote handler.

Signed-off-by: bwplotka <[email protected]>

* Make Write message type more flexble, address some feedback (#1710)

* Address remaining feedback

Signed-off-by: Saswata Mukherjee <[email protected]>

* Make Write message type more flexible

Signed-off-by: Saswata Mukherjee <[email protected]>

---------

Signed-off-by: Saswata Mukherjee <[email protected]>

* Move remote write API to client_golang/exp (#1711)

* Move remote write API to client_golang/exp

Signed-off-by: Saswata Mukherjee <[email protected]>

* Don't use api.Client structs, add options for middleware

Signed-off-by: Saswata Mukherjee <[email protected]>

* Fix reqBuf usage

Signed-off-by: Saswata Mukherjee <[email protected]>

* Fix url path

Signed-off-by: Saswata Mukherjee <[email protected]>

* Add separate mod file (and workspace file)

Signed-off-by: Saswata Mukherjee <[email protected]>

* Hook exp tests fmt; Test handler error case; Configure backoff

Signed-off-by: Saswata Mukherjee <[email protected]>

---------

Signed-off-by: Saswata Mukherjee <[email protected]>

* exp: Add README, address feedback, use sync.Pool (#1747)

* Implement suggestion for Store interface and contentType

Signed-off-by: Saswata Mukherjee <[email protected]>

* Add README

Signed-off-by: Saswata Mukherjee <[email protected]>

* Use sync.Pool

Signed-off-by: Saswata Mukherjee <[email protected]>

* Implement review suggestions

Signed-off-by: Saswata Mukherjee <[email protected]>

* Release bufs right after compressPayload

Signed-off-by: Saswata Mukherjee <[email protected]>

---------

Signed-off-by: Saswata Mukherjee <[email protected]>

* Bump exp to go 1.22

Signed-off-by: Saswata Mukherjee <[email protected]>

---------

Signed-off-by: bwplotka <[email protected]>
Signed-off-by: Saswata Mukherjee <[email protected]>
Signed-off-by: Bartlomiej Plotka <[email protected]>
Co-authored-by: Saswata Mukherjee <[email protected]>
ying-jeanne pushed a commit to ying-jeanne/client_golang that referenced this pull request Mar 28, 2025
@bwplotka @saswatamcode @ying-jeanne
exp/api: Add experimental exp module; Add remote API with write clien…
42237d5 
…t and handler. (prometheus#1658)

* api: Add remote API with write client; add remote handler.

Signed-off-by: bwplotka <[email protected]>

* Make Write message type more flexble, address some feedback (prometheus#1710)

* Address remaining feedback

Signed-off-by: Saswata Mukherjee <[email protected]>

* Make Write message type more flexible

Signed-off-by: Saswata Mukherjee <[email protected]>

---------

Signed-off-by: Saswata Mukherjee <[email protected]>

* Move remote write API to client_golang/exp (prometheus#1711)

* Move remote write API to client_golang/exp

Signed-off-by: Saswata Mukherjee <[email protected]>

* Don't use api.Client structs, add options for middleware

Signed-off-by: Saswata Mukherjee <[email protected]>

* Fix reqBuf usage

Signed-off-by: Saswata Mukherjee <[email protected]>

* Fix url path

Signed-off-by: Saswata Mukherjee <[email protected]>

* Add separate mod file (and workspace file)

Signed-off-by: Saswata Mukherjee <[email protected]>

* Hook exp tests fmt; Test handler error case; Configure backoff

Signed-off-by: Saswata Mukherjee <[email protected]>

---------

Signed-off-by: Saswata Mukherjee <[email protected]>

* exp: Add README, address feedback, use sync.Pool (prometheus#1747)

* Implement suggestion for Store interface and contentType

Signed-off-by: Saswata Mukherjee <[email protected]>

* Add README

Signed-off-by: Saswata Mukherjee <[email protected]>

* Use sync.Pool

Signed-off-by: Saswata Mukherjee <[email protected]>

* Implement review suggestions

Signed-off-by: Saswata Mukherjee <[email protected]>

* Release bufs right after compressPayload

Signed-off-by: Saswata Mukherjee <[email protected]>

---------

Signed-off-by: Saswata Mukherjee <[email protected]>

* Bump exp to go 1.22

Signed-off-by: Saswata Mukherjee <[email protected]>

---------

Signed-off-by: bwplotka <[email protected]>
Signed-off-by: Saswata Mukherjee <[email protected]>
Signed-off-by: Bartlomiej Plotka <[email protected]>
Co-authored-by: Saswata Mukherjee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kakkoyun kakkoyun kakkoyun approved these changes

@bwplotka bwplotka bwplotka approved these changes

@ArthurSens ArthurSens Awaiting requested review from ArthurSens

@vesari vesari Awaiting requested review from vesari

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@saswatamcode @kakkoyun @bwplotka