Prometheus encoder added similar to hostd #1068
Conversation
…topilot prometheus metrics.
| @@ -175,6 +176,34 @@ func (ap *Autopilot) Handler() http.Handler { | |||
| }) | |||
| } | |||
|
|
|||
| func (ap *Autopilot) writeResponse(c jape.Context, code int, resp any) { | |||
There was a problem hiding this comment.
Can you move the content of this to a function in a new file api/utils.go like so
func WriteResponse(c jape.Context, code int, resp any) errorand then call that function in here, check its error, and log it? Right now there is a lot of duplicate code.
| enc := prometheus.NewEncoder(c.ResponseWriter) | ||
| if err := enc.Append(v); err != nil { | ||
| ap.logger.Error("failed to marshal prometheus response", zap.Error(err)) | ||
| return |
There was a problem hiding this comment.
You return here without calling c.Error.
autopilot/prometheus.go
Outdated
There was a problem hiding this comment.
I'd move all your prometheus.go files into a single api/prometheus.go and implement the interface directly on the types. The types in api/*.go are supposed to be the types that get returned by the API and casting to a new type breaks that promise in some sense. Plus we can get rid of the casting altogether this way.
| @@ -701,7 +730,7 @@ func (ap *Autopilot) stateHandlerGET(jc jape.Context) { | |||
| return | |||
| } | |||
|
|
|||
| jc.Encode(api.AutopilotStateResponse{ | |||
| ap.writeResponse(jc, http.StatusOK, AutopilotStateResp(api.AutopilotStateResponse{ | |||
There was a problem hiding this comment.
This won't work with our static code analysis tool jape. It expects jc.Encode to be called in the handler right now. So without updating that this won't make it through the CI.
There was a problem hiding this comment.
We should seriously consider extending jape to support this or maybe try and write some kind of middleware for it. It won't work otherwise. Maybe we can make jape.Context an interface and then we can swap it (using a middleware) with one that's capable of handling prometheus response types. E.g. test the interface in Encode on some kind of interface that implements toPrometheus and handle it accordingly
|
|
||
| type AutopilotStateResp api.AutopilotStateResponse | ||
|
|
||
| func (asr AutopilotStateResp) PrometheusMetric() (metrics []prometheus.Metric) { |
There was a problem hiding this comment.
I'd implement PrometheusMetric or maybe ToPrometheus on the actual return type in the api package within a single api/prometheus.go. That way we don't need 3 of these prometheus.go files with duplicated types.
If a return type doesn't exist yet because it's directly returning a types.Currency or something it's fine to create a new one in the api package.
There was a problem hiding this comment.
@n8maninger wrote this I can'ti would run it by him before I make any changes to it.
| var resp interface{} | ||
| err = json.Unmarshal([]byte(setting), &resp) | ||
| if err != nil { | ||
| jc.Error(fmt.Errorf("couldn't unmarshal the setting, error: %v", err), http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| jc.Encode(resp) | ||
| if key == "s3authentication" { |
There was a problem hiding this comment.
why only return the s3authentication setting as a prometheus metric?
There was a problem hiding this comment.
the other data points are already being collected elsewhere for the other settings so it wasn't needed.
| @@ -175,6 +176,34 @@ func (ap *Autopilot) Handler() http.Handler { | |||
| }) | |||
| } | |||
|
|
|||
| func (ap *Autopilot) writeResponse(c jape.Context, code int, resp any) { | |||
There was a problem hiding this comment.
hey I think @n8maninger wrote this method not sure what it does I just use it
| return | ||
| } | ||
|
|
||
| type ContractsResp []api.ContractMetadata |
There was a problem hiding this comment.
What happens if you define PrometheusMetric on api.ContractMetadata. Wouldn't it automatically work for slices of the type too? This is a little add and when we move this PrometheusMetric methods to the api package it would be nice if we could avoid adding custom types for everything we return slices of.
|
@bustedware do you think you'll have some time this month to update your PR? |
|
@peterjan no I don't have time. this effort officially expired in January |
|
Closing this for now since it is no longer maintained. |
Continuation of #1068 Addressed some of the concerns in the reviews: - Move `PrometheusMetric` method to original type where possible - Move `writeResponse` logic to api package - Improve handling slices of Prometheus marshallable types - Disable type checking in `jape`
Prometheus encoder added to renterd similar to hostd.
?response=prometheusadded for various endpoints