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

Mixin: Add alloy cluster label filters to dashboards #2018

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gaantunes
Copy link
Contributor

PR Description

This is including the new cluster_name label introduced here to all dashboards.

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@gaantunes gaantunes requested a review from a team as a code owner November 1, 2024 17:22
@gaantunes
Copy link
Contributor Author

@wildum WDYT about this? I have noticed you have only included this label to the alloy_config_hash, but I think we could gain from having it included in every metric when this config option is set, not only on the k8s world but everywhere.

else
|||
label_values(prometheus_remote_storage_sent_batch_duration_seconds_sum{%(filterSelector)s, job="$job", instance=~"$instance", component_id=~"$component"}, url)
||| % $._config,
Copy link
Contributor

Choose a reason for hiding this comment

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

all this branching is not scaling very well. If we add another variable it will just look awful. Isn't there a way to make this simpler? I'm not familiar with the dashboards config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will consult @v-zhuravlev on this, since he is a jsonnet sorcerer.

@gaantunes
Copy link
Contributor Author

@wildum any comments regarding the including of cluster_name label to all metrics? This PR assumes that is the case.

@thampiotr
Copy link
Contributor

Could we instead recommend that if users have two or more Alloy clusters, then it's important to make them have a different job label value, so they can be distinguished easily?

I think that would be cleaner then having an ever-increasing list of labels to group by.

@gaantunes
Copy link
Contributor Author

Could we instead recommend that if users have two or more Alloy clusters, then it's important to make them have a different job label value, so they can be distinguished easily?

I think that would be cleaner then having an ever-increasing list of labels to group by.

There is currently no way to support it on integrations, unfortunately. The standard we apply for all integrations is a static job label and a platform specific cluster label that is spread to all metrics.

We currently use this static job label to correlate logs and metrics, and collect usage data to point ARR.

@gaantunes
Copy link
Contributor Author

Honestly besides the cluster label I do not see we adding any new one.

Copy link
Contributor

This PR has not had any activity in the past 30 days, so the needs-attention label has been added to it.
If you do not have enough time to follow up on this PR or you think it's no longer relevant, consider closing it.
The needs-attention label signals to maintainers that something has fallen through the cracks. No action is needed by you; your PR will be kept open and you do not have to respond to this comment. The label will be removed the next time this job runs if there is new activity.
Thank you for your contributions!

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.

3 participants