api: provide per metric metadata by gotjosh · Pull Request #6420 · prometheus/prometheus

gotjosh · 2019-12-06T16:26:44Z

This adds a new endpoint that provides per metric metadata via the V1 API. Similarly to the api/v1/target/metadata the metadata is only available for the lifecycle of the attached target.

It collapses metadata that is equal across all targets, and aggregates under the same metric name the ones that differ.

An example of a payload looks like:

curl -s http://localhost:9090/api/v1/metadata?limit=2 | jq
{
  "status": "success",
  "data": {
    "cortex_ring_tokens_total": [
      {
        "type": "gauge",
        "help": "Number of tokens in the ring",
        "unit": ""
      }
    ],
    "jaeger_tracer_sampler_updates_total": [
      {
        "type": "counter",
        "help": "Number of times the Sampler succeeded to retrieve and update sampling strategy",
        "unit": ""
      }
    ]
  }
}

I don't think there should be anything highly controversial on what I'm doing but I'll refrain from creating the docs until this is approved.

The next PR coming shortly after, will implement the same endpoint for a specific metric.

Follow up from #6409
Part of #6256

TODO:

update documentation
squash commits

brian-brazil · 2019-12-06T16:32:30Z

web/api/v1/api.go

You shouldn't need any custom marshalling code, and this doesn't appear to be doing anything non-standard.

Without it, our JSON encoding library complains about it with unsupported map key type: v1.metadataSet

Then use a string as the key.

Sorry if this not a great question, but wouldn't I still need to have custom marshalling code to define how the string should output?

No, if you think you need that then it is your data structure which needs improvement.

To layer it properly, we needed it in a different function anyway.

Not really, it's only a few lines of code. But if a different function ends up containing those lines of code I can live with that.

convertToMapOfSlicesForJSONMarshaling

Who said anything about JSON? The current endpoint functions don't known that JSON is involved, it could just as well be going to YAML or XML. That we wish to flatten a map into its values isn't related to the marshalling or JSON themselves.

But it is also used throughout this file at various places.

Only for overall marshalling, not inside any of the functions for specific endpoints. There is currently a separation of concerns, which this PR would remove.

As the name says, MarshalJSON is specific to JSON marshaling. And the method is not specific to an endpoint but to the data structure. We are essentially doing the same with promql.Point, just that this data structure is not from this package, so adding a method to it would be inappropriate.

Converting to a map of slices rather than a map of sets is specific to JSON (as JSON has no sets).

If I had infinite time, I'd happily continue our little argument, but we are way beyond diminishing returns here. I suggest once more to merge this as is.

As the name says, MarshalJSON is specific to JSON marshaling.

That's my argument, you'd need to do the exact same thing if it were YAML or most other formats. Thus why this is a smell.

If it's inside the function endpoint then it'd work for everything.

We are essentially doing the same with promql.Point, just that this data structure is not from this package, so adding a method to it would be inappropriate.

Not quite, and that's on an extremely important hot path (query/query_range) that I couldn't do a breaking change to when I was optimising that. That's one of the reasons I don't want any MarshallJSON, so that this doesn't end up needing more handcoded marshalling in the future. Nor do I want a bad pattern being introduced that other things then follow.

Using MarshallJSON is possibly less performant too, compared to letting jsoniter just handle all of this in the usual way.

I suggest once more to merge this as is.

I don't believe that code health concerns should be ignored because consensus hasn't been found. I think a strong argument is needed to introduce an inconsistent pattern which also has been known to be problematic in the past, and the arguments presented thus far do not approach that bar.

Who said anything about JSON? The current endpoint functions don't know that JSON is involved, it could just as well be going to YAML or XML.

Think this makes a great point. The current functions don't know anything about how the output should be rendered. The introduction of MarshalJSON breaks this pattern.

I'll amend it.

The last commit contains the changes made. Can I get (hopefully) one last look? 🙏

web/api/v1/api.go

web/api/v1/api_test.go

docs/querying/api.md

web/api/v1/api.go

beorn7 · 2019-12-09T18:37:19Z

web/api/v1/api.go

Sooo… the metadataSet has to be the way it is to de-duplicate the metadata, see L847 and following below. There is no way it can be "improved". Go has no native sets, so we use a map with the metadata struct as a key and "nothing" (i.e. an empty struct) as a value. So far so good.

Obviously, we cannot just marshal that metadataSet, as the complex key is not compatible with JSON. Plus, we don't even want that representation. We want a JSON array.

Now there are two possibilities:

Take the whole, possible very large metadataSet and copy it into a newly created []metadata. That would naturally marshal into exactly what we want.

Create a custom MarshalJSON to teach the metadataSet to marshal into what we want.

(1) is probably a bit less code but also less efficient. (2) is the most efficient solution. I'd intuitively prefer (2) but I don't feel strongly. @gotjosh implemented (2). I am inclined to follow that path. Not sure, though, if the code of MarshalJSON could be improved? I'm not an expert in writing those methods.

web/api/v1/api.go

docs/querying/api.md

brian-brazil · 2019-12-10T09:15:15Z

web/api/v1/api.go

You can move this into metricMetadata.

web/api/v1/api.go

beorn7

Besides the English grammar nit in L671.

gotjosh · 2019-12-10T11:39:41Z

The current failure is actually expected, I'm working on making the limit test deterministic.

docs/querying/api.md

brian-brazil · 2019-12-10T11:41:18Z

web/api/v1/api.go

That wouldn't be a layering violation, as you're not reaching into the internals of the JSON-related code or making inappropriate assumptions about how other layers work. It's the same way all standard JSON marshalling works.

However this code does contain layering violation, as it's using jsoniter which means it's depending on a particular JSON marshaller which is a bit of a smell.

I still want to avoid any use of MarshalJSON where it's not absolutely necessary. For example here the function is returning a map of maps, yet somehow the API returns a map of arrays. This is unneeded magic.

populate it in two nested for loops.

I don't see a problem with making things explicit.

This adds a new endpoint that provides per metric metadata via the V1 API. It collapses metadata that is equal across all targets, and aggregates under the same metric name the ones that differ. Signed-off-by: gotjosh <josue@grafana.com>

Signed-off-by: gotjosh <josue@grafana.com>

Some tests e.g. limit on API responses, don't require an assertion on equality. This allows us to assert against response length instead of equality. Signed-off-by: gotjosh <josue@grafana.com>

- Remove the metadataSet type. No longer needed. - Avoids using the `MarshalJSON` function. Signed-off-by: gotjosh <josue@grafana.com>

brian-brazil · 2019-12-10T14:56:21Z

I've been sitting waiting on the tests to pass for 30 minutes ;)

gotjosh · 2019-12-10T14:56:59Z

Thanks @brian-brazil! 🎉

beorn7 · 2019-12-10T14:58:37Z

web/api/v1/api.go

+		}
+	}
+
+	res := map[string][]metadata{}


I'd add a comment that this is needed for marshaling.

I'll add it in the next one which should be up soon.

beorn7

If @gotjosh likes it that way more now, that's fine with me, too. (I still disagree with the line of argument, but I don't insist on discussing it endlessly until I get it my way or get convinced myself. For that, this particular aspect is just not important enough.)

beorn7 · 2019-12-10T15:08:20Z

@brian-brazil after we had an explicit disagreement here, it would have been much nicer to wait for my response instead of just merging.

brian-brazil · 2019-12-10T15:11:35Z

I incorrectly figured that you'd want to get it in, rather than dragging it on further.

There's also some missing periods on the ends of comments in the tests added here.

beorn7 · 2019-12-10T15:13:18Z

You correctly figured that I don't want to drag it on further. But just waiting for my "OK then" would not have hurt.

gotjosh force-pushed the metrics-api branch from 9f9f3a7 to edd64ac Compare December 6, 2019 16:27

brian-brazil reviewed Dec 6, 2019

View reviewed changes

beorn7 self-requested a review December 6, 2019 17:47

gotjosh force-pushed the metrics-api branch from d12a3d0 to e357874 Compare December 9, 2019 12:46

brian-brazil reviewed Dec 9, 2019

View reviewed changes

docs/querying/api.md Outdated Show resolved Hide resolved

beorn7 requested changes Dec 9, 2019

View reviewed changes

beorn7 reviewed Dec 9, 2019

View reviewed changes

web/api/v1/api.go Outdated Show resolved Hide resolved

brian-brazil reviewed Dec 10, 2019

View reviewed changes

gotjosh force-pushed the metrics-api branch from 658187d to 49ce843 Compare December 10, 2019 10:13

beorn7 approved these changes Dec 10, 2019

View reviewed changes

brian-brazil reviewed Dec 10, 2019

View reviewed changes

gotjosh force-pushed the metrics-api branch from 06e6d46 to 14b1e4c Compare December 10, 2019 14:32

gotjosh added 4 commits December 10, 2019 14:37

api: provide per metric metadata

9120eed

This adds a new endpoint that provides per metric metadata via the V1 API. It collapses metadata that is equal across all targets, and aggregates under the same metric name the ones that differ. Signed-off-by: gotjosh <josue@grafana.com>

Add the API documentation

05d6f22

Signed-off-by: gotjosh <josue@grafana.com>

Allow tests to be asserted on response length

432061a

Some tests e.g. limit on API responses, don't require an assertion on equality. This allows us to assert against response length instead of equality. Signed-off-by: gotjosh <josue@grafana.com>

Address review comments

7c8ff2f

- Remove the metadataSet type. No longer needed. - Avoids using the `MarshalJSON` function. Signed-off-by: gotjosh <josue@grafana.com>

gotjosh force-pushed the metrics-api branch from 14b1e4c to 7c8ff2f Compare December 10, 2019 14:38

brian-brazil merged commit 0a0a228 into prometheus:master Dec 10, 2019

beorn7 reviewed Dec 10, 2019

View reviewed changes

beorn7 approved these changes Dec 10, 2019

View reviewed changes

gotjosh mentioned this pull request Dec 10, 2019

api: provide metadata for a specified metric #6442

Merged

Conversation

gotjosh commented Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gotjosh Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gotjosh Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

beorn7 left a comment

Choose a reason for hiding this comment

Uh oh!

gotjosh commented Dec 10, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brian-brazil commented Dec 10, 2019

Uh oh!

gotjosh commented Dec 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beorn7 left a comment

Choose a reason for hiding this comment

Uh oh!

beorn7 commented Dec 10, 2019

Uh oh!

brian-brazil commented Dec 10, 2019

Uh oh!

beorn7 commented Dec 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gotjosh commented Dec 6, 2019 •

edited

Loading

gotjosh Dec 6, 2019 •

edited

Loading

gotjosh Dec 9, 2019 •

edited

Loading