Skip to content

Conversation

@pipiland2612
Copy link
Contributor

@pipiland2612 pipiland2612 commented Oct 28, 2025

What issue is this PR fixing?

Changes

Since this is a very large work, I'd be happy to hear everyone opinions on this. Thanks for your time!

Signed-off-by: pipiland2612 <[email protected]>
Signed-off-by: pipiland2612 <[email protected]>
Signed-off-by: pipiland2612 <[email protected]>
Signed-off-by: pipiland2612 <[email protected]>
Signed-off-by: pipiland2612 <[email protected]>
Signed-off-by: pipiland2612 <[email protected]>
Signed-off-by: pipiland2612 <[email protected]>
Signed-off-by: pipiland2612 <[email protected]>
Signed-off-by: pipiland2612 <[email protected]>
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.

Thanks! Looks great from the high level, so far.

Small question, I will try to run it and check in details in the following days. Perhaps @saswatamcode or Juraj have time too

Signed-off-by: pipiland2612 <[email protected]>
Signed-off-by: pipiland2612 <[email protected]>
compliance tests for PromQL correctness. It's designed for smaller in-place quick unit tests, e.g. on per-PR basis, using docker based test framework. Useful as an acceptance tests
for vendors or those who wish to maintain high Prometheus compatibility over time.

## Remote Write: Sender
Copy link
Member

Choose a reason for hiding this comment

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

TODO(to myself or others): Link receiver while we are at it.

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.

Nice work!

Some comments, but generally great! I didn't review all tests in details, but some early thoughts:

  • This is good. Let's directly base it to main and do changes now, it's good!
  • Let's fix index.html (: (receiver mention when I ran sender)
image
  • Let's either update or remove circle CI that expects sender. Ideally we rely only on GH actions.
  • Let's move sender/receiver README to a single readme under remotewrite. Those are really similar (share same logic). Also when you run index.html It tries local ./result.json now and it makes no sense UNLESS we write shared docs for both sender/receiver.
  • Some of the tests are neither MUST, SHOULD or MAY - they are not in the spec e.g. optimizations like symbols must be deduplicated. Maybe it's obvious but maybe we need "OPTIMIZATION" rfc level?
{"Time":"2025-11-07T15:36:31.437028Z","Action":"pass","Package":"github.com/prometheus/compliance/remotewrite/sender","Elapsed":467.59}
Image

@pipiland2612
Copy link
Contributor Author

pipiland2612 commented Nov 8, 2025

Hi @bwplotka, thanks for reviewing.

Nice work!

Some comments, but generally great! I didn't review all tests in details, but some early thoughts:

  • This is good. Let's directly base it to main and do changes now, it's good!
  • Let's fix index.html (: (receiver mention when I ran sender)

Let's either update or remove circle CI that expects sender. Ideally we rely only on GH actions.

Let's move sender/receiver README to a single readme under remotewrite. Those are really similar (share same logic). Also when you run index.html It tries local ./result.json now and it makes no sense UNLESS we write shared docs for both sender/receiver.

Some of the tests are neither MUST, SHOULD or MAY - they are not in the spec e.g. optimizations like symbols must be deduplicated. Maybe it's obvious but maybe we need "OPTIMIZATION" rfc level?

  1. Hmm strange, my index.html show the right thing:
Screenshot 2025-11-08 at 12 59 26
  1. I have removed the sender from the circle CI test: 248bad3

  2. Move receiver/sender readme.md under cd3f5d2

  3. Changing the test rfcLevel: 15874dd

PTAL thanks!

@pipiland2612 pipiland2612 requested a review from bwplotka November 8, 2025 13:28
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.

Looks amazing! I still need to look deeper thought.

Can we change the base to main?

Can we fix CI? (remove circleCI?)

The test suite automatically downloads and runs Prometheus as the reference sender implementation. For testing custom senders, place the binary in the `bin/` directory.

### For Receiver Tests
A Prometheus server with Remote-Write Receiver enabled, as baseline:
Copy link
Member

Choose a reason for hiding this comment

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

This also requires Go 1.23+ no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I dont know, probably not as the receiver readme.md doesn't mention this.

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.

.

dir:
- alert_generator
- promql
- remotewrite/sender
Copy link
Member

Choose a reason for hiding this comment

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

We could create (copy) linting GH action from other Prometheus repos quickly and fix it.

@bwplotka bwplotka changed the base branch from exp-sender to main November 14, 2025 15:05
@bwplotka
Copy link
Member

Change base of this PR to main

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.

Left some comments, so far!

Slowly going through (:

Signed-off-by: pipiland2612 <[email protected]>
// RW 1.0 format doesn't have created_timestamp field.
// If sender is truly in RW 1.0 mode, this field should be 0/unset.
for _, ts := range req.Request.Timeseries {
should(t, int64(0) == ts.CreatedTimestamp, "RW 1.0 should not use created_timestamp field")
Copy link
Member

Choose a reason for hiding this comment

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

To be updated with Prometheus main

Copy link
Member

Choose a reason for hiding this comment

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

ping (: @pipiland2612

I would love to merge this compliance framework 👍🏽

Copy link
Contributor Author

@pipiland2612 pipiland2612 Nov 19, 2025

Choose a reason for hiding this comment

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

Hi @bwplotka, thanks for pinging me. Solved at 386931a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

remote_write_sender: Add support for Remote Write 2.0

2 participants