Validate the table name in NewTable #52
Merged
+51
−7
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
To avoid constructing table names which are not usable for metrics
(e.g. as Prometheus metrics label), restrict the allowed name to
"^[a-z][a-z0-9_\\-]{1,30}$".
Signed-off-by: Jussi Maki <[email protected]>
joamaki
force-pushed
the
pr/joamaki/validate-table-names
branch
from
4c65b74 to
84a92b9
Compare
|
ovidiutirla
approved these changes
Sep 6, 2024
| malformed, for example if `IDIndex` is not unique (primary index has to be), or if | ||
| the indexers have overlapping names. | ||
| more secondary indexes. The table name must match the regular expression | ||
| "^[a-z][a-z0-9_\\-]{0,30}$". |
There was a problem hiding this comment.
Should we allow _ or should we enforce - to be used always?
Is there a particular reason for selecting 30? 30 chars -> ~4-8 words.
Considering a metric name needs to be usually below 200 chars, but the table name could be used in situation like jobgroup prefix -> table name -> suffix, I think it's more than enough.
The largest table name we currently have is 16 chars, bandwidth-qdiscs which IMO is fine.
There was a problem hiding this comment.
There's two parts to the names in the job library. One is the name used in the module health and that is concat of the full module id (agent.control-plane.foo) and the job name (job-control-loop). The module ids are already validated, but indeed we don't validate the job name (e.g. the name argument passed to job.OneShot for example).
Would you be interested in doing a PR for cilium/hive to add this validation? Probably same or similar constraint as used here makes sense.
There was a problem hiding this comment.
Should we allow _ or should we enforce - to be used always?
I think allowing both is fine and using a similar constraint as we use in e.g. cilium/hive for module ids makes it easier to guess what is allowed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To avoid constructing table names which are not usable for metrics (e.g. as Prometheus metrics label), restrict the allowed name to "^[a-z][a-z0-9_\-]{0,30}$", e.g. must be lower-case, start with letter and be between 1-31 characters long (is this long enough limit?)