Skip to content

feat: prom rw exporter add support for rw2 #38235

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

Conversation

jmichalek132
Copy link
Contributor

@jmichalek132 jmichalek132 commented Feb 26, 2025

Description

PR for adding rw2 support in the prometheus remote write exporter.

Restart of a PR #35888

Link to tracking issue #33661

Fixes

Signed-off-by: Juraj Michalek <[email protected]>
@jmichalek132 jmichalek132 marked this pull request as ready for review March 4, 2025 21:28
@jmichalek132 jmichalek132 requested a review from a team as a code owner March 4, 2025 21:28
@jmichalek132
Copy link
Contributor Author

Tests failing due to go leak on go.opencensus.io asked about it in this issue #30438 (comment)

Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

Great start! I know we previously talked about how it was OK to duplicate a lot of code between v1 and v2 because we would quickly clean it up with follow-up PRs. Seeing how difficult it was for us all to find time for this, it may be wise not to duplicate so much. What do you think?

Also, for the goleak test failures, you can add this to the metadata file and run the Makefile shenanigans :)

tests:
config:
goleak:
ignore:
top:
# See https://github.com/census-instrumentation/opencensus-go/issues/1191 for more information.
- "go.opencensus.io/stats/view.(*worker).start"

@@ -57,6 +58,9 @@ type Config struct {

// SendMetadata controls whether prometheus metadata will be generated and sent
Copy link
Member

Choose a reason for hiding this comment

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

Should we update this comment to mention that this boolean flag only applies to v1?

@@ -50,7 +50,7 @@ The following settings can be optionally configured:
- *Note the following headers cannot be changed: `Content-Encoding`, `Content-Type`, `X-Prometheus-Remote-Write-Version`, and `User-Agent`.*
- `namespace`: prefix attached to each exported metric name.
- `add_metric_suffixes`: If set to false, type and unit suffixes will not be added to metrics. Default: true.
- `send_metadata`: If set to true, prometheus metadata will be generated and sent. Default: false.
- `send_metadata`: If set to true, prometheus metadata will be generated and sent. Default: false. This option is ignored when using PRW 2.0, which already includes metadata.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `send_metadata`: If set to true, prometheus metadata will be generated and sent. Default: false. This option is ignored when using PRW 2.0, which already includes metadata.
- `send_metadata`: If set to true, prometheus metadata will be generated and sent. Default: false. This option is ignored when using PRW 2.0, which always includes metadata.

Comment on lines +137 to +139
if err := config.RemoteWriteProtoMsg.Validate(cfg.RemoteWriteProtoMsg); err != nil {
return nil, err
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused about why we need this here. Don't we validate the Proto Message when in config.Validate() already? I'm not sure when the config validation happens though 🤔

Comment on lines +760 to +767
if tt.enableSendingRW2 {
cfg.RemoteWriteProtoMsg = config.RemoteWriteProtoMsgV2
oldValue := enableSendingRW2FeatureGate.IsEnabled()
testutil.SetFeatureGateForTest(t, enableSendingRW2FeatureGate, tt.enableSendingRW2)
defer testutil.SetFeatureGateForTest(t, enableSendingRW2FeatureGate, oldValue)
} else {
cfg.RemoteWriteProtoMsg = config.RemoteWriteProtoMsgV1
}
Copy link
Member

Choose a reason for hiding this comment

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

We can get rid of the else here, easier to read :)

cfg.RemoteWriteProtoMsg = config.RemoteWriteProtoMsgV1
if tt.enableSendingRW2 {
    cfg.RemoteWriteProtoMsg = config.RemoteWriteProtoMsgV2
.
.
}

Comment on lines +33 to +35
// exportV2 sends a Snappy-compressed writev2.Request containing writev2.TimeSeries to a remote write endpoint.
func (prwe *prwExporter) exportV2(ctx context.Context, requests []*writev2.Request) error {
input := make(chan *writev2.Request, len(requests))
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I'm still not sure how to do it, but it's not looking very nice that we're duplicating a lot of code between v1 and v2 now 😬

The exportv2 function is quite lengthy; it might be hard to maintain both of them in the long run. Have we explored other ideas?

Copy link
Member

Choose a reason for hiding this comment

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

They are literally the same, actually. The only difference is the signature 🤔, could we pass a byte array and a boolean that signals which RW version it represents?

I'm also open for other ideas in case anyone has any

@atoulme atoulme marked this pull request as draft March 17, 2025 22:34
@atoulme
Copy link
Contributor

atoulme commented Mar 17, 2025

Please resolve conflicts and address feedback and mark the PR as ready for review. Thanks!

Copy link
Contributor

github-actions bot commented Apr 1, 2025

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Apr 1, 2025
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants