scrape,api: provide per-target metric metadata #4183

fabxc · 2018-05-22T18:07:09Z

This adds a per-target cache of scraped metadata. The metadata is only
available for the lifecycle of the attached target. An API endpoint allows
to select metadata by metric name and a label selection of targets.

Per my (so far short-running) test I somewhat surprisingly cannot detect any difference in memory or CPU utilization. (edit: okay, let's not jump to conclusions, the test input is probably heavily skewed.)

I'll throw in an API-level test once we have agreement on it. It's kinda tedious due to some tech debt.

API example:

curl -G http://localhost:9091/api/v1/targets/metadata \
    --data-urlencode 'match=go_goroutines{job="prometheus"}' \
    --data-urlencode 'limit=3' | jq
{
  "status": "success",
  "data": [
    {
      "target": {
        "instance": "127.0.0.1:9090",
        "job": "prometheus"
      },
      "type": "gauge",
      "help": "Number of goroutines that currently exist."
    },
    {
      "target": {
        "instance": "127.0.0.1:9091",
        "job": "prometheus"
      },
      "type": "gauge",
      "help": "Number of goroutines that currently exist."
    },
    {
      "target": {
        "instance": "localhost:9090",
        "job": "prometheus"
      },
      "type": "gauge",
      "help": "Number of goroutines that currently exist."
    }
  ]
}

I think providing metric and target label matchers through a single series matcher is quite elegant and aligns well with our other endpoints. Possibly we could ignore matchers on labels a target doesn't have – this is a bit one-off semantics but would allow clients to just pass entire series. Since they cannot know which labels are target labels, this seems useful in practice.
Otherwise, just passing instance and job matchers seems relatively safe.

@brian-brazil @jkohen

jkohen

LGTM. We agreed to add a LIST method to dump the metadata for all metrics, as an optimization for the startup case. The current implementation makes it easy to add it later, so no worries.

jkohen · 2018-05-22T20:38:09Z

scrape/scrape.go

+
+// metaEntry holds meta information about a metric.
+type metaEntry struct {
+	lastIter uint64


Can you document this field? It's not obvious to me.

jkohen · 2018-05-22T20:41:18Z

scrape/scrape.go

+	}
+	c.metaMtx.Lock()
+	for m, e := range c.metadata {
+		if c.iter-e.lastIter > 10 {


Any comments on how 10 was picked?

I want a reasonable amount of buffer time where we can still catch metadata after a metric has officially disappeared. With normal scrape intervals, 10 will give us 3 to 10 minutes.
I'd do even more personally. But I think @brian-brazil advocated for being rather strict about it.

I was talking mostly about targets going away. Metrics should ~never go away once they are exposed on a target, so this isn't buying you anything.

I misunderstood you then. My initial version did not have any garbage collection but I added this based on your comment.
Now that we have it, it probably makes sense to just keep it. Metrics should never disappear, but this will help if clients do something funky.

It's inconsistent with the the others, which is a bit weird. Needs a comment at the very least.

jkohen · 2018-05-22T20:54:18Z

scrape/scrape.go

+	c.metaMtx.Unlock()
+}
+
+func (c *scrapeCache) getMetadata(metric string) (textparse.MetricType, string, bool) {


What's your position on creating a struct type for the metadata, instead of passing MetricType/string separately? If we add a field at some point, having a type will make it easier to change the code.

We'll probably need that anyway for a list API. So that'll likely happen.

jkohen · 2018-05-22T21:13:51Z

scrape/scrape.go


 	// seriesCur and seriesPrev store the labels of series that were seen
 	// in the current and previous scrape.
 	// We hold two maps and swap them out to save allocations.
 	seriesCur  map[uint64]labels.Labels
 	seriesPrev map[uint64]labels.Labels
+
+	metaMtx  sync.Mutex


An RWMutex could prove beneficial here.

The hold time is very short everywhere. Based on past experience in those scenarios, the RWMutex performs notably worse then. Especially since >99% of the time we will need the write lock.

It's also unlikely we'll have more than one reader. In which case all callers are mutually exclusive anyway.

jkohen · 2018-05-22T21:15:42Z

scrape/target.go

+	if t.metadataFunc == nil {
+		return textparse.MetricTypeUnknown, "", false
+	}
+	return t.metadataFunc(metric)


You probably want to either unlock before calling this function or wrap with RLock(), or concurrency for this API will effectively be limited to 1.

If you unlock, consider switching the RWMutex to a regular Mutex or an atomic pointer, which are lighter options for this use.

Never mind, the concurrency limit is for the target, not the API, so probably not important. I think it's still cleaner to either call RLock or switch to Mutex.

Yes, that makes sense.

fabxc · 2018-05-22T23:25:01Z

In a bigger test with 1000 mysql_exporter targets, we are looking at 5-7% memory usage increase. CPU remains unchanged. That still seems fairly good.

In practice I'd expect it to be a bit less if anything. All scraped test series are constant, artificially lowering the baseline memory usage. There's also an unusually high amount of single-series metrics.

brian-brazil

Just a quick look. I'm not sure how this works naming/api wise with listing metadata.

brian-brazil · 2018-05-24T12:42:19Z

web/api/v1/api.go

+	if s := r.FormValue("limit"); s != "" {
+		var err error
+		if limit, err = strconv.Atoi(s); err != nil {
+			return nil, &apiError{errorBadData, fmt.Errorf("limit must be a  number")}


double space before number

brian-brazil · 2018-05-24T12:48:13Z

pkg/textparse/parse.go

@@ -237,6 +237,7 @@ const (
 type MetricType string

 const (
+	MetricTypeUnknown   = ""


This should have untyped as a value (or unknown if you want to future-proof for openmetrics).

Yes, missed untyped intitially but have this added locally as yet another one. Untyped is known and unknown. I wanted MetricTypeUnknown for unknown unknown.

Within the current format untyped is that, there is no "unknown".

Fair, fine with using a boolean in places where that doesn't quite cut it.

What cases are you thinking of?

With the new changes it's no longer relevant I suppose.

brian-brazil · 2018-05-24T12:49:41Z

scrape/scrape.go

+	}
+	c.metaMtx.Lock()
+	for m, e := range c.metadata {
+		if c.iter-e.lastIter > 10 {


I was talking mostly about targets going away. Metrics should ~never go away once they are exposed on a target, so this isn't buying you anything.

brian-brazil · 2018-05-24T12:51:57Z

web/api/v1/api.go

+		}
+	}
+	if metric == "" {
+		return nil, &apiError{errorBadData, errors.New("matcher must specify metric name")}


I don't think this should be required, you should be able to dump all metadata for one target.

Yes, was just about to add that as a "list" implementation that's still per-target.
After some consideration, a global list will generate massive amounts of data and seems to dangerous.

Yea, this requires some thought but it'd still be very useful to get an (arbitrarily) deduped list.

I'd opt for providing the metadata first and waiting for feature requests. Better than providing something now with guessed semantics and risky performance implications.

brian-brazil · 2018-05-24T12:52:19Z

web/api/v1/api.go

+	}
+
+	var metric string
+	for i, m := range matchers {


A comment that this is extracting/removing the metric name would be useful

brian-brazil · 2018-05-24T13:10:01Z

scrape/scrape.go

 	}
 }

 func (c *scrapeCache) iterDone() {
-	// refCache and lsetCache may grow over time through series churn
+	// All caches may grow over time through series churn


I thought we'd fixed that?

I merely changed the comment since it was a bit outdated. This just points out why we are doing the garbage collection below.

fabxc · 2018-05-24T15:27:20Z

Listing functionality and endpoint documentation added.

krasi-georgiev · 2018-05-25T04:54:52Z

just curious what is the use case for this metadata?

brian-brazil · 2018-06-01T15:06:45Z

docs/querying/api.md

+
+URL query parameters:
+
+- `match=<series_selector>`: A selector that matches targets by label matchers. If a metric name is provided, only metadata for that metric name is returned.


You should be clear about what the label matchers are matching against

I'm wondering now if we should split the metric name from the target labels URL parameter wise, it's a bit unclean to have them as one

Fine with me. metric and match_target ok? target_matcher would work too but be inconsistent with our other endpoints with match params.

brian-brazil · 2018-06-01T15:08:05Z

docs/querying/api.md

@@ -363,6 +363,85 @@ $ curl http://localhost:9090/api/v1/targets
 }
 ```

+## Target metadata
+
+The following endpoint returns metadata about metrics currently scraped by targets.


This should be marked experimental for now

brian-brazil · 2018-06-01T15:08:34Z

docs/querying/api.md

+}
+```
+
+The following example returns metadata for all metrics for the target with


for all the targets

brian-brazil · 2018-06-01T15:14:13Z

scrape/scrape.go

+	defer c.metaMtx.Unlock()
+
+	for m, e := range c.metadata {
+		res = append(res, MetricMetadata{


Can we pre-initilise this to be the right size?

brian-brazil · 2018-06-01T15:14:52Z

scrape/scrape.go

+			sl.cache.setType(p.Type())
+			continue
+		case textparse.EntryHelp:
+			sl.cache.setHelp(p.Help())


Where is the Help getting unescaped? I didn't see it in the lexing code.

This is problematic since we'd allocate a new string. If we store it unescaped, we'd need to unescape all help strings on each scrape and do the same. If we store escaped and unescaped version, our memory usage for this feature doubles, since it's virtually all in help strings.

We could only do it on demand when reading the help strings via the API – that will also come with heavy allocs, but is certainly better.
Only for everything to get escaped all over again when we return it as JSON...

Any ideas on how to handle this all?

There's also no generic unescape function AFAICS. So we have to do replacements ourselves. Is there a full list of escaped symbols we've to cover?

https://golang.org/pkg/strconv/#Unquote

We've already got code for this for label values, might be worth seeing how it works. The vast majority of strings won't need escaping.

For help strings it's only \ and \n.

Yes, I realized it shortly after I posted. The docs don't quite give it away, the docs for the symmetric Quote are clearer. It expects actual quotes at beginning and end though, which means we need to allocate yet another transient string to add those.

Why would \t not be allowed?

Ah right, we did the whole thing for label values already. The notable difference is that it's called only on first sight a series there. We'd be doing it on every scrape – so if there's escaped stuff in there, the impact is notably bigger.

Tabs can be represented by themselves, so no need to escape them.

It seems rather unusual. I don't think I've encountered anything in the wild yet the opted to only escape some things but not others. And I'm relatively sure it's gonna throw off more common escape handling (strconv.Unquote will just error for example) – after all this parser is not the only consumer.

I guess that raises questions about all other escape sequences that our current handling in label values doesn't cover either.

If that's the case then those parsers are buggy. \t for example should end up as a backslash followed by a t.

This adds a per-target cache of scraped metadata. The metadata is only available for the lifecycle of the attached target. An API endpoint allows to select metadata by metric name and a label selection of targets. Signed-off-by: Fabian Reinartz <freinartz@google.com>

Signed-off-by: Fabian Reinartz <freinartz@google.com>

brian-brazil

👍

brian-brazil · 2018-06-06T12:31:06Z

scrape/scrape.go

+
+// metaEntry holds meta information about a metric.
+type metaEntry struct {
+	lastIter uint64 // last scrape iteration the entry was observed


Comment format is off here

brian-brazil · 2018-06-06T12:32:27Z

scrape/scrape.go

+			sl.cache.setType(p.Type())
+			continue
+		case textparse.EntryHelp:
+			sl.cache.setHelp(p.Help())


If that's the case then those parsers are buggy. \t for example should end up as a backslash followed by a t.

Signed-off-by: Fabian Reinartz <freinartz@google.com>

jacobsy · 2018-10-25T01:33:17Z

Method Not Allowed

jkohen reviewed May 22, 2018

View reviewed changes

brian-brazil reviewed May 24, 2018

View reviewed changes

fabxc force-pushed the metastore branch 2 times, most recently from 5d704f0 to d418e31 Compare May 24, 2018 15:26

fabxc force-pushed the metastore branch 2 times, most recently from 2fad6f7 to 91b6e86 Compare May 24, 2018 17:20

fabxc mentioned this pull request May 25, 2018

pkg/textparse: refactor and add metadata handling #4162

Merged

fabxc force-pushed the scrapemeta branch 4 times, most recently from 86e2c85 to 76a4a46 Compare May 25, 2018 15:28

fabxc force-pushed the metastore branch from 91b6e86 to 6b78af6 Compare May 25, 2018 15:43

brian-brazil reviewed Jun 1, 2018

View reviewed changes

fabxc changed the base branch from scrapemeta to master June 5, 2018 10:08

fabxc force-pushed the metastore branch 3 times, most recently from ae96914 to 0d6739e Compare June 6, 2018 09:21

Fabian Reinartz added 2 commits June 6, 2018 05:56

pkg/textparse: unescape help string

0ff42e7

Signed-off-by: Fabian Reinartz <freinartz@google.com>

fabxc force-pushed the metastore branch from 0d6739e to 053423a Compare June 6, 2018 09:56

brian-brazil reviewed Jun 6, 2018

View reviewed changes

Address comments

057a5ae

Signed-off-by: Fabian Reinartz <freinartz@google.com>

fabxc force-pushed the metastore branch from 053423a to 057a5ae Compare June 6, 2018 15:22

fabxc merged commit e4bb422 into master Jun 6, 2018

fabxc deleted the metastore branch June 6, 2018 16:23

brian-brazil mentioned this pull request Jun 13, 2018

Store metrics docstrings and allow to query them #2747

Closed

beorn7 mentioned this pull request Nov 29, 2019

Proposal: Add API endpoint to retrieve metadata by metric name #6395

Closed


		URL query parameters:

		- `match=<series_selector>`: A selector that matches targets by label matchers. If a metric name is provided, only metadata for that metric name is returned.

scrape,api: provide per-target metric metadata #4183

scrape,api: provide per-target metric metadata #4183

Conversation

fabxc commented May 22, 2018 • edited

jkohen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabxc May 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabxc May 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabxc commented May 22, 2018 • edited

brian-brazil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabxc commented May 24, 2018

krasi-georgiev commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabxc Jun 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabxc Jun 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-brazil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobsy commented Oct 25, 2018

fabxc commented May 22, 2018 •

edited

fabxc May 24, 2018 •

edited

fabxc May 22, 2018 •

edited

fabxc commented May 22, 2018 •

edited

fabxc Jun 4, 2018 •

edited

fabxc Jun 4, 2018 •

edited