-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
api/v1: added /status/runtimeinfo #5165
Conversation
As these are specific to an API interface and should not break once a different API (web.go) interface changes, I would vote for duplicating the structs in I am not a maintainer of Prometheus. What do others think? |
Hmm, how about factoring that out and providing a warning that the contents of the map may change over time? |
@brian-brazil By factoring out do you mean to place the structs in a different file which both api.go and web.go can import? |
Yes
…On Fri 1 Feb 2019, 23:23 Vishnunarayan K I ***@***.*** wrote:
@brian-brazil <https://github.com/brian-brazil> By factoring out do you
mean to place the structs in a different file which both api.go and web.go
can import?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5165 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGyTdroGjj8OYL9KZ0Ih2MQcMZ4-pP45ks5vJMxagaJpZM4acrl4>
.
|
1326c57
to
3f93820
Compare
@brian-brazil I factored it out. Can you check the implementation? I'll add the warning and necessary docs after that. |
I have added the documentation of the api (with a warning it might change) and the required tests. |
289c745
to
3d504e6
Compare
Resolved all issues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a consistent API is important. I am sorry for the nit picking.
@mxinden All fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit. Other than that this looks good to me. Are there any thoughts by others left?
@mxinden Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks for bearing with us @vn-ki!
Giving @brian-brazil the last vote as a Prometheus maintainer.
web/web_types/types.go
Outdated
CWD string `json:"cwd"` | ||
Version *PrometheusVersion `json:"version"` | ||
Alertmanagers []*url.URL `json:"alertmanagers"` | ||
GoroutineCount int `json:"goroutineCNT"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a particular reason why you specifically abbreviate Count
-> CNT
here? Seems inconsistent with the other fields, especially the other ...Count
fields.
Otherwise this PR looks 👍
web/web_types/types.go
Outdated
GOGC string `json:"goGC"` | ||
CorruptionCount int64 `json:"corruptionCount"` | ||
ChunkCount int64 `json:"chunkCount"` | ||
TimeSeriesCount int64 `json:"timeSeriesCount"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Count here still implies a counter, which this isn't. And chunks, goroutines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't have made that association because it's not a metric name, but don't care either way.
On the other hand you might ask if we really need to offer data over this API that we already have on /metrics as well (goroutines, number of time series, etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand you might ask if we really need to offer data over this API that we already have on /metrics as well (goroutines, number of time series, etc.).
I feel like providing an API makes those a bit more accessible.
Should I remove it?
web/web_types/types.go
Outdated
"time" | ||
) | ||
|
||
// RunRuntimeInfo contains runtime info about Prometheus. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RunRuntimeInfo -> RuntimeInfo
docs/querying/api.md
Outdated
"corruptionCount": 0, | ||
"goGC": "", | ||
"goMaxProcs": 4, | ||
"goroutineCNT": 29, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would need to be updated to align with Julius' comment
web/web_types/types.go
Outdated
Birth time.Time `json:"startTime"` | ||
CWD string `json:"cwd"` | ||
Version *PrometheusVersion `json:"version"` | ||
Alertmanagers []*url.URL `json:"alertmanagers"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be a slice of strings instead of url.URL
. This is the current API output with 1 configured AlertManager:
{
"status": "success",
"data": {
...
"alertmanagers": [
{
"Scheme": "http",
"Opaque": "",
"User": null,
"Host": "localhost:9093",
"Path": "/api/v1/alerts",
"RawPath": "",
"ForceQuery": false,
"RawQuery": "",
"Fragment": ""
}
],
...
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@simonpasquier The status.html
template uses the Host
, Path
and Scheme
.
I'm not sure what to do. Any help will be appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I would be inclined to declare an intermediary Alertmanager
struct that would only expose the required fields.
type AlertManager struct {
Scheme string `json:"scheme"`
Host string `json:"host"`
Path string `json:"path"`
}
type RuntimeInfo struct {
...
Alertmanagers []Alertmanager `json:"alertmanagers"`
...
}
302f32d
to
19eaa6d
Compare
@simonpasquier Done and rebased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me. A few nits on the documentation.
docs/querying/api.md
Outdated
@@ -683,6 +683,46 @@ $ curl http://localhost:9090/api/v1/status/flags | |||
|
|||
*New in v2.2* | |||
|
|||
### Runtime Info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Information
docs/querying/api.md
Outdated
@@ -683,6 +683,46 @@ $ curl http://localhost:9090/api/v1/status/flags | |||
|
|||
*New in v2.2* | |||
|
|||
### Runtime Info | |||
|
|||
The following endpoint returns runtime info of the Prometheus server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/info/information/ too
docs/querying/api.md
Outdated
{ | ||
"status": "success", | ||
"data": { | ||
"alertmanagers": null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to add a non-null value.
...
"alertmanagers": [
{
"scheme": "http",
"host": "localhost:9093",
"path": "/api/v1/alerts"
}
],
...
Signed-off-by: Vishnunarayan K I <appukuttancr@gmail.com>
@simonpasquier Done |
@vn-ki you have conflicts to resolve. |
We have this upstream already in the React UI (https://prometheus.demo.do.prometheus.io/api/v1/status/runtimeinfo). Thanks for working on this btw! |
This does not yet build. I want some pointers before I can go ahead.
Right now, because I have the types in
web.go
, I am running into a cyclic import.Is there a file where I can place
RuntimeInfo
struct andPrometheusVersion
struct?If not, how do I tackle this problem? Is my approach fundamentally wrong?
Ping: @brancz @mxinden
Closes #2467
Signed-off-by: Vishnunarayan K I appukuttancr@gmail.com