Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iterating http_sd support towards generic discovery #84

Closed
mrnetops opened this issue Dec 4, 2021 · 21 comments
Closed

iterating http_sd support towards generic discovery #84

mrnetops opened this issue Dec 4, 2021 · 21 comments

Comments

@mrnetops
Copy link
Contributor

mrnetops commented Dec 4, 2021

I believe we can make http_sd work generically out of the box without relabel_configs.

by explicitly setting the __params_target label per target (and ideally using the host header and port from the request to set the target:port) we can get service discovery to work without any jiggery pokery.

I used https://github.com/pagarme/static-response-server to host the following

[
  {
    "targets": [
      "fastly-exporter:8080"
    ],
    "labels": {
      "__param_target": "0AizkuJPvMmqhulU7fXXXX"
    }
  },
  {
    "targets": [
      "fastly-exporter:8080"
    ],
    "labels": {
      "__param_target": "0KO5PPKDAMlzAQ22fsXXXX"
    }
  }
]

along with the following minimal http_sd_configs

  - job_name: 'fastly-exporter'

    http_sd_configs:
            - url: http://static-content:7070

and everything worked out of the box.

@peterbourgon
Copy link
Contributor

by explicitly setting the __params_target label per target (and ideally using the host header and port from the request to set the target:port) we can get service discovery to work without

How should the fastly-exporter change?

@peterbourgon
Copy link
Contributor

I used https://github.com/pagarme/static-response-server to host the following . . .

It should not be necessary to run a separate process in order to use the fastly-exporter with http_sd_config.

@mrnetops
Copy link
Contributor Author

mrnetops commented Dec 4, 2021

I used https://github.com/pagarme/static-response-server to host the following . . .

It should not be necessary to run a separate process in order to use the fastly-exporter with http_sd_config.

Oh, that was literally just to test that the new http_sd endpoint json worked the way I wanted without having to diddle the golang code first.

I had already gotten your relabel_configs variant working first before looking to see how we could iterate it.

@peterbourgon
Copy link
Contributor

Oh! OK, sorry for presuming :)

@peterbourgon
Copy link
Contributor

peterbourgon commented Dec 4, 2021

So instead of /sd returning

[
	{
		"targets": [
			"abc",
			"def"
		]
	}
]

you want

[
	{
		"targets": [
			"localhost:8080"
		],
		"labels": {
			"__param_target": "abc"
		}
	},
	{
		"targets": [
			"localhost:8080"
		],
		"labels": {
			"__param_target": "def"
		}
	}
]

Is that right? If it works, this avoids some configuration in the prometheus.yaml — does it do anything beyond that?

Unfortunately, I'm not sure it can actually work — I don't think the exporter can serve a target host:port which is reliably correct. Consider the case of load balancers, or NAT, or anything which puts a middlebox between the client and the exporter. The host header isn't a reliable source of truth. I've fought this battle many times actually!! As far as I can see it's a properly hard problem for a process to know it's own address in a way that permits it to serve that address back callers reliably.

@peterbourgon
Copy link
Contributor

/cc @SuperQ if you have any input

@mrnetops
Copy link
Contributor Author

mrnetops commented Dec 4, 2021

ok, I was thinking we might have the problems you mentioned with the host header, so...

use the ip:port that is being connected to, as seen by the fastly-exporter, as the target?

that addresses at least some of the cases and for more complex ones folks can still do relabel_configs to fix up if needed.

not great, but possibly better? we REALLY need a prometheus placeholder for http_sd that means "come back to the hostname you asked"

targets": [ "__meta_http_sd_url_host" ],

Which obviously CAN be done with relabel_configs but I'd love to have it available implicitly so things work out of the box.

@peterbourgon
Copy link
Contributor

peterbourgon commented Dec 4, 2021

It would be lovely if it were possible! But I'm not aware of a way for a server to reliably deduce the address that a client used to establish an incoming connection. NAT, VPNs, proxies, etc. can all sit in front of the connection in a way that's invisible to the server. Middleboxes can intercept and rewrite traffic at the IP layer. DNS resolution happens before the initial handshake. And so on.

@peterbourgon
Copy link
Contributor

Unfortunately not feasible.

@mrnetops
Copy link
Contributor Author

mrnetops commented Dec 4, 2021

Well, wait a sec. you're right that there are a myriad of problems detecting from the server side, but in this case we would just need to provide a hint to the client, who does have enough information to make the right decision.

ala, "hey client, connect to me like last time"

So we just need to convince the http_sd folks to implicitly handle the hint on the prometheus side... ;)

@mrnetops
Copy link
Contributor Author

mrnetops commented Dec 4, 2021

As an interim step on our side, we can do as you mocked up as that at least reduces the complexity of the relabel_configs while we negotiate with the http_sd folks

    relabel_configs:
      - source_labels: [__address__]
        target_label: __param_target
      - source_labels: [__param_target]
        target_label: service
      - target_label: __address__
        replacement: 127.0.0.1:8080

to

    relabel_configs:
      - target_label: __address__
        replacement: 127.0.0.1:8080

"service": "<service>" can optionally be added to the /sd labels as well if we still want that explicit label

@peterbourgon
Copy link
Contributor

peterbourgon commented Dec 4, 2021

We can't do what I mocked up. "localhost:8080" is not (necessarily) a valid target.

@mrnetops
Copy link
Contributor Author

mrnetops commented Dec 4, 2021

it should be, no? assuming just about every system under the sun has a localhost -> 127.0.0.1 lookup.

potential alternate suggestions:

  • 127.0.0.1:8080
  • fastly-exporter:8080

as really the target field (for fastly-exporter) is currently more used as a placeholder text string for replacement then an actual host/ip for binding

i.e. in our intermediate implementation, we don't need the target to be resolvable, we just need it to be consistent to be replaceable with relabel_configs ;)

@mrnetops
Copy link
Contributor Author

mrnetops commented Dec 4, 2021

...aaaand I have a snippet from my first attempt to not have to update the address in two places (http_sd_configs -> url and __address__)

              - source_labels: [__meta_url]
                regex: '.*://(.+)/.*'
                target_label: __address__
                replacement: ${1}

slightly wordier, but keeps up to date with whatever host:port is used in http_sd_configs -> url via __meta_url so folks don't have to update two places.

That actually makes it irrelevant what the target is actually called in the /sd json ;)

@peterbourgon
Copy link
Contributor

What about that? #85

@peterbourgon
Copy link
Contributor

...aaaand I have a snippet . . .

Does this actually scrape each service correctly?

@peterbourgon
Copy link
Contributor

Upon reflection, I don't think it's the job of the exporter to try to golf it's SD implementation to minimize the required configuration on the Prometheus side. I think we just do what Prometheus says we should do and leave it at that.

@mrnetops
Copy link
Contributor Author

mrnetops commented Dec 5, 2021

I'm not a fan of golf either (code or otherwise. possibly miniature?)

So, let me take the 10,000 foot view and explain what I'm beating this gong so hard

automated discovery of service discovery endpoints

About a year from now, folks are going to be like

Hey, http_sd is nifty (it is!) I'd like to autodiscover service discovery endpoints (not targets, endpoints) like what is available with /metrics endpoints in kubernetes for example.

ala

kubernetes 
-> [autodiscover service discovery] <- hypothetical, but likely
-> [service discovery endoint(s) (e.x. fastly-exporter/sd)]
-> [target(s) (e.x. fastly-exporter-1/metrics)]

Now, /metrics autodiscovery is awesome as /metrics works out of the box. Any /metrics endpoint is processed the exact same way. ...However, currently fastly-exporter service discovery needs application specific relabel_configs transforms to work.

Part of this onus is going to be on projects (like fastly-exporter) implementing http_sd, but I think the other part is going to be on the dhttp_sd docs and examples requiring service discovery endpoints work out of the box without relabel_configs by default. Otherwise, if autodiscover is attempted with them, they will not work.

Because otherwise fastly-exporter and other implementers are going to have a gnarly time of it that we can more easily nip in the bud early.

This is why I think it is so absolutely important that fastly-exporter /sd work out of the box, not with less relabel_configs (tho I did provide examples), but with none.

@peterbourgon
Copy link
Contributor

These are points to raise on the Prometheus repo, not this one, I think :)

@mrnetops
Copy link
Contributor Author

mrnetops commented Dec 5, 2021

oh, I'm poking them as well ;) I also wanted to clarify why I'm not letting this go for this project as well.

@SuperQ
Copy link
Contributor

SuperQ commented Dec 5, 2021

Sorry, I didn't get a chance to review this quickly.

I don't think the exporter can serve a target host:port which is reliably correct.

Yes, this is going to be the tough part. For example, in our configuration we're going to have several exporters running different shards, with semi-dynamic hostnames generated by Kubernetes.

I haven't actually gotten a chance to test the existing implementation yet, as I am working on adding additional scrape target injection into our Kubernetes magement. I think that should be done soon, so I'll hopefully be able to report back before the end of the year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants