Skip to content
This repository has been archived by the owner on Nov 5, 2021. It is now read-only.

[Feature Request] Add support for creating metrics from HTTP probe output #297

Open
manugarg opened this issue Oct 24, 2019 · 8 comments
Open

Comments

@manugarg
Copy link
Contributor

Cloudprober supports building additional metrics (other than the default ones) from external probe output. We could possible do the same for HTTP probe.

@mfboulos
Copy link

mfboulos commented Nov 9, 2019

Mind if I take this one? I just learned Go and I'm eager to contribute.

@manugarg
Copy link
Contributor Author

@mfboulos That's excellent.

Logic for this should look very similar to how we do it for external probes:
https://github.com/google/cloudprober/blob/master/probes/external/payload_metrics.go

In fact, we should try to refactor the config and code in such a way that we can use them for both -- HTTP and external probe. I have a pending change to move parseValue from payload_metrics.go to metrics package, so it will be better to wait for that change (by Tue). I'll also think about sharing config and code in the meantime.

@mfboulos
Copy link

That makes sense, configuration and metrics should be decoupled from the probe types. I'll take the time to familiarize myself with the workflow and such.

@manugarg
Copy link
Contributor Author

I think external and payload parsing logic are sufficiently decoupled now. External probe initializes a payload parser at the time initialization:

p.payloadParser, err = payload.NewParser(p.c.GetOutputMetricsOptions(), "external", p.name, metrics.Kind(defaultKind), p.l)

It updates payload metrics per target using this parser:

result.payloadMetrics = p.payloadParser.PayloadMetrics(result.payloadMetrics, ps.payload, ps.target)

I think we'd want to do something similar for HTTP probes.

@mfboulos
Copy link

Sorry it took me so long to get around to this, I've been working on a personal project.

Anyways, I'm having a bit of trouble with a couple things. Considering I've never used cloudprober at all, it's a bit difficult for me to grasp what's going on throughout, so bear with me.

It feels like the payload metric parsing is still somewhat tightly coupled with the external probe, since PayloadMetrics takes the same format as expected from the payload in an external probe result. This seems to differ from the http probe in a way that I would have to write another method in the payload parser and modify the http proto to accommodate for that.

If I'm missing something that indicates that they're indeed not as tightly coupled as I'm seeing, then it may be due to the fact that I'm just not entirely sure what metrics we're passing from the http probe results to begin with.

Any further clarity on those fronts would help me immensely in resolving this issue.

@manugarg
Copy link
Contributor Author

payloadParser.PayloadMetrics expects payload to just be a text that looks like this:

For external probe it's produced by either running an external command:

payload: string(b),
,

or by sending a probe request to probe server:

payload: rep.GetPayload(),

In case of HTTP probe, payload will be HTTP response body in the similar format that I described above. Currently we are not parsing HTTP response body (except for the option -- export_response_as_metrics).

(Just FYI: I am going to be away from work for next 6 days. I'll read your response after that.)

@mfboulos
Copy link

Alright, that makes sense! I've added a couple optional fields to the config proto to mirror the changes on the external probe, which should allow configuration with the same level of freedom.

However, I'm having a bit of trouble with protoc that I can't resolve. With the same metrics payload proto import as the external probe config proto, protoc doesn't want to resolve the import, regardless of whether or not I set the proto path to a directory that can reach both protos.

This is pretty much the last blocker before I fully hash this out.

@manugarg
Copy link
Contributor Author

There is a script in tools directory to generate protobuf code, did you give that a try?

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

Successfully merging a pull request may close this issue.

2 participants