Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tremes The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
saswatamcode
left a comment
saswatamcode
left a comment
There was a problem hiding this comment.
Thanks, this is quite better already, just some comments
pkg/prometheus/guardrails.go
Outdated
| var ( | ||
| // guard rails with their default values | ||
| disallowExplicitNameLabel = &Guardrail{Name: GuardrailDisallowExplicitNameLabel, RequireTSDBEndpoint: false, Value: true} | ||
| requireLabelMatcher = &Guardrail{Name: GuardrailRequireLabelMatcher, RequireTSDBEndpoint: false, Value: true} | ||
| disallowBlanketRegex = &Guardrail{Name: GuardrailDisallowBlanketRegex, RequireTSDBEndpoint: true, Value: true} | ||
| maxMetricCardinality = &Guardrail{Name: GuardrailMaxMetricCardinality, RequireTSDBEndpoint: true, Value: 20000} | ||
| maxLabelCardinality = &Guardrail{Name: GuardrailMaxLabelCardinality, RequireTSDBEndpoint: true, Value: 500} | ||
| ) |
There was a problem hiding this comment.
How about creating a NewGuardrail constructor for these instead of global.
There was a problem hiding this comment.
Maybe event maintaing them in a type, with get/set methods. Looks like we already have set methods :)
There was a problem hiding this comment.
This way, when a flag for no tsdb endpoint is passed, we just don't init the guardrails, and instead create no-op guardrails on that field.
So the check would just be a passthrough in IsSafeQuery, and we don't need to keep checking
There was a problem hiding this comment.
+1 for the constructor, although not 100% sure if I understand the rest. Do you mean something like:
func NewDefaultGuardrails(hasTSDB bool) Guardrails
and check/get the TSDB availability in the caller (loader.go)?
2 participants
No description provided.