add mesos slave monitor/statics api #3494
Conversation
|
Is there anything more to be added? |
|
I won't have time to review this until next month, I know we need to look into the problems this code caused when it was originally added and see if we can provide the option to get these metrics without causing issues with the database. This code was disabled in #1686 |
danielnelson
added
the
feat
| @@ -63,7 +65,7 @@ var sampleConfig = ` | |||
| # "system", | |||
| # "executors", | |||
| # "tasks", | |||
| # "messages", | |||
| # "messages", | |||
There was a problem hiding this comment.
Remove trailing space from this line
| @@ -367,7 +369,7 @@ func getMetrics(role Role, group string) []string { | |||
| ret, ok := m[group] | |||
|
|
|||
| if !ok { | |||
| log.Printf("I! [mesos] Unknown %s metrics group: %s\n", role, group) | |||
| log.Printf("I! [mesos] Unkown %s metrics group: %s\n", role, group) | |||
There was a problem hiding this comment.
Revert back to proper spelling of Unknown
| jf.Fields["executor_id"] = task.ExecutorID | ||
|
|
||
| acc.AddFields("mesos_tasks", jf.Fields, tags, timestamp) | ||
| acc.AddFields("mesos_monitor_statics", jf.Fields, tags, timestamp) |
There was a problem hiding this comment.
What's your reasoning for changing the measurement name from mesos_tasks to mesos_monitor_statistics?
| @@ -326,7 +329,7 @@ func TestMesosSlave(t *testing.T) { | |||
| m := Mesos{ | |||
| Masters: []string{}, | |||
| Slaves: []string{slaveTestServer.Listener.Addr().String()}, | |||
| // SlaveTasks: true, | |||
| //SlaveMonitorStatics: true, | |||
There was a problem hiding this comment.
Was this meant to be uncommented?
|
Why not parse all information about mesos/state?? |
|
As Daniel mentioned, the code to parse all information about mesos/state was commented out because it seemed to be causing problems. I reviewed the PR though so you can address those comments and update your branch and we'll be that much closer to merging if the issues become less. |
|
@glinton or @danielnelson sorry to bring up an old stale PR but is there any technical problems with re-enabling per task granularity for Mesos now that TSI exists? |
|
I'm okay with this going forward, we should probably just put a warning in the README like we did for |
|
@jcmartins please let us know when you've addressed the requested changes and fixed the failing checks and conflicts. |
|
@jcmartins Checking in if this is something you'd like to finish or to see if anyone else would like to take this over, thanks. |
|
Closing this issue due to inactivity. If you would still like this plugin, please submit it as an external plugin that can be used with |
sjwang90
added
closed/external-candidate
external plugin
Required for all PRs: