MON-3500: Enable sending exemplars over RW in UWM #2161

rexagod · 2023-11-23T00:02:03Z

I added CHANGELOG entry for this change.
No user facing changes, so no entry in CHANGELOG was needed.

openshift-ci · 2023-11-23T00:02:08Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2023-11-23T00:02:14Z

@rexagod: This pull request references MON-3500 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.15.0" version, but no target version was set.

In response to this:

I added CHANGELOG entry for this change.

No user facing changes, so no entry in CHANGELOG was needed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/manifests/types.go

rexagod · 2023-11-27T08:15:43Z

/retest

pkg/manifests/manifests.go

pkg/manifests/types.go

pkg/manifests/manifests.go

simonpasquier · 2023-11-27T08:53:39Z

pkg/manifests/manifests.go

+		// Explicitly warn that we are ignoring the `SendExemplars` configuration for in-cluster scenarios.
+		for _, rws := range f.config.ClusterMonitoringConfiguration.PrometheusK8sConfig.RemoteWrite {
+			if rws.SendExemplars != nil && *rws.SendExemplars {
+				klog.Warningln("Enabling `SendExemplars` in the in-cluster Prometheus configuration has no affect, as it is only supported for UWM configurations.")


I'm not sure we need a warning log. IMHO it would be lost in the existing output + there's no harm being done by having the flag enabled here.

Agree on this, maybe just adding some DEBUG info in case we'd like to check it out.

ACK. I thought that even though there's no weight to this being enabled here (since exemplar-storage is not enabled), we might want to have an indication of that behavior. Nonetheless, I agree that it's fairly easy for someone to figure out if any experimental feature is not enabled by looking over at the CLI args supplied to the binary.

I'll drop this. 👍🏼

@danielmellado Ah, it seems I missed your comment. Do you suppose we replace Warningln with a V(x) level call?

simonpasquier

It needs an e2e test, at least to check that the feature gets enabled.
Ideally we add exemplars support in https://github.com/rhobs/prometheus-example-app and check that they can be queried via the Prometheus/Thanos APIs.

pkg/manifests/types.go

simonpasquier

/hold
/cc @bburt-rh
waiting for tests and Brian's review.

rexagod · 2023-11-28T07:53:27Z

/test e2e-agnostic-operator

test/e2e/config_test.go

pkg/manifests/types.go

bburt-rh · 2023-11-28T15:35:09Z

/label docs-approved

simonpasquier · 2023-11-28T16:06:52Z

/hold cancel
/lgtm

openshift-ci · 2023-11-28T16:20:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rexagod, simonpasquier

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rexagod,simonpasquier]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2023-11-28T17:30:20Z

/retest-required

Remaining retests: 0 against base HEAD 958ddf0 and 2 for PR HEAD 1f1db96 in total

jan--f · 2023-11-28T18:12:07Z

pkg/manifests/types.go

+	// configured to store a maximum of 100,000 exemplars in memory.
+	// Note that this setting only applies to user-defined monitoring. It is not applicable
+	// to default in-cluster monitoring.
+	SendExemplars *bool `json:"sendExemplars,omitempty"`


I though we could also have a

type UWMRemoteWriteSpec struct { RemoteWriteSpec // Enables sending exemplars via remote write. When enabled, Prometheus is // configured to store a maximum of 100,000 exemplars in memory. SendExemplars *bool `json:"sendExemplars,omitempty"` }

and then use that in PrometheusRestrictedConfig. That would make it clearer what option can be used where. Though not sure how that affects the API doc generation.

just to be sure, this is not a blocker for me and we could go ahead as is.

openshift-ci · 2023-11-28T19:28:11Z

@rexagod: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-ovn-single-node	`1f1db96`	link	false	`/test e2e-aws-ovn-single-node`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2023-11-28T23:12:48Z

[ART PR BUILD NOTIFIER]

This PR has been included in build cluster-monitoring-operator-container-v4.15.0-202311282232.p0.gfbd3e7c.assembly.stream for distgit cluster-monitoring-operator.
All builds following this will include this PR.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 23, 2023

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 23, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 23, 2023

rexagod force-pushed the 3500 branch from bcc255a to e3d94d6 Compare November 23, 2023 00:05

simonpasquier reviewed Nov 23, 2023

View reviewed changes

pkg/manifests/types.go Outdated Show resolved Hide resolved

rexagod force-pushed the 3500 branch 4 times, most recently from ed24b98 to b6ea201 Compare November 26, 2023 15:35

rexagod marked this pull request as ready for review November 26, 2023 15:36

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 26, 2023

openshift-ci bot requested review from danielmellado and marioferh November 26, 2023 15:36

rexagod force-pushed the 3500 branch from b6ea201 to ff907f8 Compare November 26, 2023 15:37

MON-3500: Enable sending exemplars over RW in UWM

75569e2

rexagod force-pushed the 3500 branch from ff907f8 to 75569e2 Compare November 26, 2023 22:48

rexagod commented Nov 27, 2023

View reviewed changes

pkg/manifests/manifests.go Outdated Show resolved Hide resolved

simonpasquier reviewed Nov 27, 2023

View reviewed changes

rexagod force-pushed the 3500 branch 2 times, most recently from 3d0fb69 to 3df6b9d Compare November 27, 2023 12:23

fixup! MON-3500: Enable sending exemplars over RW in UWM

332b7f5

rexagod force-pushed the 3500 branch from 3df6b9d to 332b7f5 Compare November 27, 2023 12:23

simonpasquier reviewed Nov 27, 2023

View reviewed changes

pkg/manifests/types.go Outdated Show resolved Hide resolved

simonpasquier reviewed Nov 27, 2023

View reviewed changes

openshift-ci bot requested a review from bburt-rh November 27, 2023 15:32

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2023

simonpasquier reviewed Nov 28, 2023

View reviewed changes

test/e2e/config_test.go Outdated Show resolved Hide resolved

test/e2e/config_test.go Outdated Show resolved Hide resolved

fixup! fixup! MON-3500: Enable sending exemplars over RW in UWM

589b8d4

rexagod force-pushed the 3500 branch from 7ee4b67 to 589b8d4 Compare November 28, 2023 10:53

bburt-rh reviewed Nov 28, 2023

View reviewed changes

pkg/manifests/types.go Outdated Show resolved Hide resolved

fixup! fixup! fixup! MON-3500: Enable sending exemplars over RW in UWM

1f1db96

rexagod force-pushed the 3500 branch from 713cc43 to 1f1db96 Compare November 28, 2023 15:20

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Nov 28, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 28, 2023

openshift-ci bot assigned simonpasquier Nov 28, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2023

jan--f reviewed Nov 28, 2023

View reviewed changes

openshift-merge-bot bot merged commit fbd3e7c into openshift:master Nov 28, 2023

simonpasquier mentioned this pull request Dec 5, 2023

MON-3543: Update API docs for Metrics Server #2181

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MON-3500: Enable sending exemplars over RW in UWM #2161

MON-3500: Enable sending exemplars over RW in UWM #2161

rexagod commented Nov 23, 2023

openshift-ci bot commented Nov 23, 2023

openshift-ci-robot commented Nov 23, 2023 •

edited by openshift-ci bot

Loading

rexagod commented Nov 27, 2023

simonpasquier Nov 27, 2023

danielmellado Nov 27, 2023

rexagod Nov 27, 2023

rexagod Nov 27, 2023 •

edited

Loading

simonpasquier left a comment

simonpasquier left a comment

rexagod commented Nov 28, 2023

bburt-rh commented Nov 28, 2023

simonpasquier commented Nov 28, 2023

openshift-ci bot commented Nov 28, 2023

openshift-ci-robot commented Nov 28, 2023

jan--f Nov 28, 2023

jan--f Nov 28, 2023

openshift-ci bot commented Nov 28, 2023

openshift-bot commented Nov 28, 2023

MON-3500: Enable sending exemplars over RW in UWM #2161

MON-3500: Enable sending exemplars over RW in UWM #2161

Conversation

rexagod commented Nov 23, 2023

openshift-ci bot commented Nov 23, 2023

openshift-ci-robot commented Nov 23, 2023 • edited by openshift-ci bot Loading

rexagod commented Nov 27, 2023

simonpasquier Nov 27, 2023

Choose a reason for hiding this comment

danielmellado Nov 27, 2023

Choose a reason for hiding this comment

rexagod Nov 27, 2023

Choose a reason for hiding this comment

rexagod Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

simonpasquier left a comment

Choose a reason for hiding this comment

simonpasquier left a comment

Choose a reason for hiding this comment

rexagod commented Nov 28, 2023

bburt-rh commented Nov 28, 2023

simonpasquier commented Nov 28, 2023

openshift-ci bot commented Nov 28, 2023

openshift-ci-robot commented Nov 28, 2023

jan--f Nov 28, 2023

Choose a reason for hiding this comment

jan--f Nov 28, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Nov 28, 2023

openshift-bot commented Nov 28, 2023

openshift-ci-robot commented Nov 23, 2023 •

edited by openshift-ci bot

Loading

rexagod Nov 27, 2023 •

edited

Loading