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

fix: allow prometheus extend rule [ATMOSPHERE-48] #1215

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

Conversation

ricolin
Copy link
Member

@ricolin ricolin commented May 15, 2024

fix: #738

@ricolin ricolin requested a review from mnaser May 15, 2024 15:02
@mnaser mnaser changed the title fix: allow prometheus extend rule fix: allow prometheus extend rule [ATMOSPHERE-48] Jun 28, 2024
Copy link
Member

@mnaser mnaser left a comment

Choose a reason for hiding this comment

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

I see a few issues with this current approach.

This means that there's no way of actually editing the existing rules, this is just extending/adding to the additional existing rules. This means that this can simply be replaced with kube_prometheus_stack_helm_values.additionalPrometheusRulesMap instead which becomes just a documentation change.

I'd rather we do that than introduce a new variable for this.

@mpiscaer
Copy link
Contributor

mpiscaer commented Jun 28, 2024

@mnaser Is don't know if i understand this correctly but when you use at the combine filter use 'list_merge=append' it will extend the string and then you can use the feature from jsonnet to overide/extend the existing settings.

@ricolin
Copy link
Member Author

ricolin commented Jul 2, 2024

I see a few issues with this current approach.

This means that there's no way of actually editing the existing rules, this is just extending/adding to the additional existing rules. This means that this can simply be replaced with kube_prometheus_stack_helm_values.additionalPrometheusRulesMap instead which becomes just a documentation change.

I'd rather we do that than introduce a new variable for this.

If we need override existing, another way we I can try is to write an ansible plugin so we can handle combine on real recursive ( and avoid constraint that brought by existing combine function https://github.com/vexxhost/atmosphere/pull/1215/files#diff-2162201c39f35aed971e9b5b2fc33cb1860cfa756c8cf776683d15fe686b9cfaR291-R296 )

@ricolin ricolin requested a review from mnaser July 5, 2024 06:57
@ricolin
Copy link
Member Author

ricolin commented Jul 5, 2024

@mnaser Is don't know if i understand this correctly but when you use at the combine filter use 'list_merge=append' it will extend the string and then you can use the feature from jsonnet to overide/extend the existing settings.

The issue with directly using list_merge=append for combine, is that will give us two rule group with same name and makes error for charts like

{ 
    "ipmi-exporter": {
        "groups": [
            {
                "name": "rules",
                "rules": [ { "alert": "IpmiCollectorDown", "expr": "ipmi_up == 0", "for": "10m", "labels": {"severity": "P3"}, } ],
            },
            {
                "name": "rules",
                "rules": [ { "alert": "IpmiCollectorDown", "expr": "ipmi_up == 0", "for": "16m", "labels": {"severity": "P2"}, } ],
            },
        ]
    }   
}

I just added a plugin to handle that error

@ricolin
Copy link
Member Author

ricolin commented Sep 27, 2024

recheck

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.

Enable extending monitoring rules for Atmosphere
3 participants