G-Research / geras

Geras provides a Thanos Store API for the OpenTSDB HTTP API. This makes it possible to query OpenTSDB via PromQL, through Thanos.
https://github.com/G-Research/geras
Apache License 2.0
38 stars 16 forks source link

Prometheus' data model requires more characters be switched #87

Closed johnseekins closed 3 years ago

johnseekins commented 3 years ago

: and - need to be removed, but other characters OpenTSDB may allow also don't work. This PR ensures names follow the data model more precisely.

johnseekins commented 3 years ago

In general, there are other patterns we should technically follow to meet the prometheus data model... These are the regexes we use for our ingest tool:

    allowedNames     = regexp.MustCompile("^[a-zA-Z_:][a-zA-Z0-9_:]*$")
    allowedFirstChar = regexp.MustCompile("[a-zA-Z]")
    allowedFields    = regexp.MustCompile("^[a-zA-Z0-9_:]*$")
    replaceChars     = regexp.MustCompile("[^a-zA-Z0-9_:]")
    allowedTagKeys   = regexp.MustCompile("[a-zA-Z][a-zA-Z0-9_]*")

There's technically limits on tag keys, too. And technically, prometheus metric "names" can't start with _. But really, this MR is probably more than enough, right?

dgl commented 3 years ago

There's technically limits on tag keys, too. And technically, prometheus metric "names" can't start with _. But really, this MR is probably more than enough, right?

Yes, in most ways Prometheus is more permissive than OpenTSDB (see tag limit of 8, which is the main reason OpenTSDB can't be a generic store for Prometheus metrics). In fact I think the only character we'd missed was /, at least according to http://opentsdb.net/docs/build/html/user_guide/query/timeseries.html which says:

Metric names, tag names and tag values have to be made of alpha numeric characters, dash “-“, underscore “_”, period “.”, and forward slash “/”, as is enforced by the package-private function Tags.validateString.

(I don't think I've seen / used very often though.)

johnseekins commented 3 years ago

I would certainly hope people aren't using / in their metric names/tag names often... BTW, you can actually increase the tag limit in OpenTSDB... tsd.storage.max_tags = 14, but the general uid limits are an equal problem.

johnseekins commented 3 years ago

I think I've resolved your concerns (and tests do pass now). As for only running the replace if the character is defined...I did add an if in for that and it seems to work.

dgl commented 3 years ago

Thanks! I have a few little test changes, but I'll merge them separately.

dgl commented 3 years ago

(Also we're quite familiar with the UID limits see, https://www.gresearch.co.uk/article/opentsdb-meta-cache-trade-offs-for-performance/ for some stats on our instance :) )

johnseekins commented 3 years ago

(Also we're quite familiar with the UID limits see, https://www.gresearch.co.uk/article/opentsdb-meta-cache-trade-offs-for-performance/ for some stats on our instance :) )

I forgot about that! We're actually using your metacache tool in our instance! Made a huge difference in performance.