Skip to content
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

feat: workload labels #1065

Merged
merged 1 commit into from
Aug 1, 2024
Merged

feat: workload labels #1065

merged 1 commit into from
Aug 1, 2024

Conversation

bernot-dev
Copy link
Collaborator

Implement go/gmp:expand-target-labels.

@bernot-dev bernot-dev self-assigned this Jul 11, 2024
@bernot-dev bernot-dev force-pushed the bernot-dev/workload-labels branch 3 times, most recently from 75d27cc to 551cacb Compare July 12, 2024 15:02
@bernot-dev bernot-dev marked this pull request as ready for review July 12, 2024 15:18
Copy link
Collaborator

@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.

Relabelling rules looks generally good, but did you intend to change AlertingConfig? Relabeling in Alerting Config only adds labels to alerts, not metrics. Wonder if you meant to do this scrape config section instead? Or am I missing something? 🤔

Tests would be nice for metrics too (unless you meant to do this for alerts).

SourceLabels: prommodel.LabelNames{"__meta_kubernetes_endpoints_name"},
Regex: svcNameRE,
})
cfg.RelabelConfigs = append(cfg.RelabelConfigs,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we ensure there is a dynamic (e.g. config based) switch off for this? e.g. when we block some customer with this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The design rejected putting this change behind a feature flag, and also suggests that the creation of a feature flag would increase confusion and the maintenance burden for TargetLabels.

An MSA is planned to notify users of this upcoming change and allow them to adjust accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

I believe the idea is that users before this change are unimpacted since there may be customers already using a label named workload_controller. The PodMonitoring.targetLabels field is essentially the switch. This PR is missing that.

@bernot-dev let's update the Defaulter, such that if the customer created a new PodMonitoring (e.g. did not set a targetLabels value), we add workload_controller in the list. Then, only add these additional configurations if it is specified (see relabelingsForMetadata).

@bernot-dev bernot-dev force-pushed the bernot-dev/workload-labels branch 2 times, most recently from e863275 to 94e6d53 Compare July 13, 2024 02:31
@bernot-dev
Copy link
Collaborator Author

did you intend to change AlertingConfig?

Nope. That was my mistake. Should be fixed now.

@bernot-dev bernot-dev force-pushed the bernot-dev/workload-labels branch 6 times, most recently from 61cf1d8 to 63c43d3 Compare July 30, 2024 20:45
Implement go/gmp:expand-target-labels.
Copy link
Collaborator

@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.

Love it, thanks! I like this to be implicit for us, so we can iterate and own this (flexibility).

Can we add test for podmonitoring config that filters this out (to check if it's possible for users, I think the agreement was that we want it).

@pintohutch
Copy link
Collaborator

FYI - we're still awaiting final word on if these labels are sufficient cc @xichen2020

@bernot-dev
Copy link
Collaborator Author

FYI - we're still awaiting final word on if these labels are sufficient cc @xichen2020

@pintohutch Is this blocking?

@pintohutch
Copy link
Collaborator

pintohutch commented Aug 1, 2024

I don't think it's blocking this PR per se. We may want to wait on final labels before a release though

@bernot-dev bernot-dev merged commit cb350d3 into main Aug 1, 2024
27 checks passed
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.

None yet

4 participants