Target ordering inconsistent when using custom metrics path

[roganartu]

What did you do?

• Set targets via file_sd_configs, where each target inside the file has the same instance but adds a unique __metrics_path__ label (for proxying metrics to devices that can't run exporters, similar to snmp_exporter)
• Refresh /targets endpoint multiple times

What did you expect to see?

• Consistent, sorted ordering of targets

What did you see instead? Under which circumstances?

• Randomly ordered targets list each refresh

Cause
Sorting by only the instance label: https://github.com/prometheus/prometheus/blob/master/web/web.go#L690-L694

Proposal
Sort by the full URL (including query params, sorted by key?) instead of just the instance

Happy to do a PR for this is the proposal is acceptable.

[brian-brazil]

To clarify, do you have targets with identical target labels?

[roganartu]

Identical targets, unique __metrics_path__, yes.

A concrete example of a targets.json file that triggers this:

[
  {
    "labels": {
      "__metrics_path__": "/foo"
    },
    "targets": [
      "metrics-proxy:1234"
    ]
  },
  {
    "labels": {
      "__metrics_path__": "/bar"
    },
    "targets": [
      "metrics-proxy:1234"
    ]
  }
]

[brian-brazil]

That's not going to work out, every target needs a unique set of target labels or you'll have problems.

I'll accept a change to sort by all target labels, but not one to sort by URL.

[roganartu]

What kinds of problems do you expect the above config to encounter? I've been running that structure for a week or so now and haven't noticed any issues aside from the broken ordering on the targets page. The scrape timings (and errors for some chronically failing targets) line up with expectations.

I'm also unsure what you mean by every target needs a unique set of target labels. The combination of target-label is unique across all the resulting targets, is that not sufficient?

Would the above config not simply result in two targets with meta labels of:

• {"__address__": "metrics-proxy:1234", "__metrics_path__": "/foo"}
• {"__address__": "metrics-proxy:1234", "__metrics_path__": "/bar"}
  This is what I seem to see in practice.

[brian-brazil]

The combination of target-label is unique across all the resulting targets, is that not sufficient?

No it isn't, there's no label distinguishing those two targets.

[roganartu]

It seems we have different definitions of labels and targets here.

There is very clearly a unique label for each target in my example json blob above, being the __metrics_path__. What you seem to be saying is that you don't consider __metrics_path__ to be a part of either the target or the label sets when comparing uniqueness. Is that accurate?

Not considering the path when comparing uniqueness but letting users modify it seems like an odd choice to me.

To clarify, from everything I can see in metrics coming out this config renaming to unique paths 100% works in practice despite your assertions to the contrary. The only thing that appears to be broken is the sorting of the target list. Is there some component or expected behaviour somewhere that leads you to believe so strongly that this doesn't work.

[brian-brazil]

__metrics_path__ is not a target label, as it begins with two underscores and is thus filtered away with all the other metadata. Your setup is not sane.

[roganartu]

Right, now we're getting somewhere.

So by target labels you mean the post-relabel configs labels.

The __metrics_path__ maps (at the collector proxy running on that metrics-proxy:1234) to a unique label (that gets relabelled to instance anyway) on scraping, so it's not like there's a bunch of targets going to the same series.

ie:

some_counter_total{instance="foo"}

and

some_counter_total{instance="bar"}

This is not that different to how snmp_exporter works, it just places the label uniqueness requirement in a different place (at the collector instead of in the targets list). Prometheus doesn't seem to care that my pre-scrape target label set isn't unique, it happily keeps the targets separate because they have different paths.

I understand why it would be better to use query params like snmp_exporter does for this, but that ship has sailed. It's not like sorting the targets by full URL makes this footgun worse, it just makes it less annoying to look at the targets.

[brian-brazil]

That still doesn't make your setup sane, look more closely at what the snmp exporter does.

[roidelapluie]

agree -- it is not sane. Just think about your "up" metrics.