Initial support for Mesos master #682

tripledes · 2016-02-11T00:11:11Z

This PR adds support for Mesos masters metrics. Let me know if anything should be change/modified.

Last commit could be reverted, although I think it could help avoiding issues with existing users when adding more mesos components.

tripledes · 2016-02-11T00:17:52Z

Sorry, missed that issue reported by the test, will fix right away.

sparrc · 2016-02-11T15:41:39Z

plugins/inputs/mesos/mesos_test.go

+	}
+}
+
+func TestSampleConfig(t *testing.T) {


you don't need to test this

sparrc · 2016-02-11T15:42:19Z

looks good, thanks @tripledes, I'll do a full review tomorrow

tripledes · 2016-02-11T16:29:58Z

@sparrc Thanks, I'll remove those tests later today.

tripledes · 2016-02-11T18:20:48Z

Hmm seems the test failed but not because of my changes :)

sparrc · 2016-02-12T18:17:52Z

Code looks good, but can you write a README based on this template?: https://github.com/influxdata/telegraf/blob/master/plugins/inputs/EXAMPLE_README.md

tripledes · 2016-02-16T08:34:00Z

@sparrc Just added the README, test seems to fail because of something else.

sparrc · 2016-02-16T18:17:12Z

@tripledes I pulled down your changes but I'm also getting the config file panic that CircleCI is hitting. I can't see anything in your code that would cause this, still investigating...

sparrc · 2016-02-16T19:03:23Z

@tripledes I'm sorry but I really can't figure out what the problem is here, when I create a sample-config file using your code, I get toml parse panic. When I simply delete the inputs.mesos table, I don't get the panic (using your code or the master code)

sparrc · 2016-02-16T19:16:22Z

these are basically the steps to reproduce:

telegraf -sample-config > telegraf.conf
telegraf -config telegraf.conf -input-filter cpu:mem -test

tripledes · 2016-02-16T20:58:46Z

@sparrc I'll have a look, thanks for taking the time to check.

tripledes · 2016-02-16T21:28:08Z

@sparrc not sure about this, but feels like a bug in the TOML parser, could you please give a try to the following config snippet?

# Telegraf plugin for gathering metrics from N Mesos masters
[[inputs.mesos]]
  # Timeout, in ms
  timeout = 100
  # A list of Mesos masters. e.g. master1:5050, master2:5050, etc.
  # The port can be skipped if using the default (5050)
  # Default value is localhost:5050.
  masters = ["localhost:5050"]
  # Metrics groups to be collected.
  master_collections = ["resources"]

As soon as I add another comment on top of master_collections or more elements to it, the parser crashes. There are currently some issues opened against naoina/toml that feel somehow related.

tripledes · 2016-02-16T21:33:44Z

@sparrc quick update, seems related to multi-line comments, leaving the sample config to the following, works:

# Telegraf plugin for gathering metrics from N Mesos masters
[[inputs.mesos]]
  # Timeout, in ms.
  timeout = 100
  # Default value is localhost:5050.
  masters = ["localhost:5050"]
  # Default, all enabled.
  master_collections = ["resources","master","system","slaves","frameworks","messages","evqueue","registrar"]

Please, give a try and let me know whether that would be enough for the sample config and I'll modify the method.

Still feels like something funny is going on in the parser.

Cheers.

sparrc · 2016-02-16T22:20:42Z

hm, definitely seems like a bug in the parser, I don't understand....there are other plugins with multi-line comments, correct? Why does this seem to only affect yours?

tripledes · 2016-02-16T22:53:56Z

Good question :)

So, different combinations:

The following ones work:

# Telegraf plugin for gathering metrics from N Mesos masters
[[inputs.mesos]]
  # Timeout, in ms.
  timeout = 100
  # Default value is localhost:5050.
  masters = ["localhost:5050"]
  # Metrics groups to be collected.
  # Default, all enabled.
  master_collections = ["resources","master","system","slaves","frameworks","messages","evqueue","registrar"]

# Telegraf plugin for gathering metrics from N Mesos masters
[[inputs.mesos]]
  # Timeout, in ms.
  timeout = 100
  # The quick brown fox jumps over the lazy dog
  masters = ["localhost:5050"]
  # The quick brown fox jumps over the lazy dog
  master_collections = ["resources","master","system","slaves","frameworks","messages","evqueue","registrar"]

# Telegraf plugin for gathering metrics from N Mesos masters
[[inputs.mesos]]
  # Timeout, in ms.
  timeout = 100
  # The quick brown fox jumps over the lazy dog
  masters = ["localhost:5050"]
  # The quick brown fox jumps over the lazy dog
  # blahblah
  master_collections = ["resources","master","system","slaves","frameworks","messages","evqueue","registrar"]

# Telegraf plugin for gathering metrics from N Mesos masters
[[inputs.mesos]]
  # Timeout, in ms.
  timeout = 100
  # The quick brown fox jumps over the lazy dog
  # blahblah
  masters = ["localhost:5050"]
  # The quick brown fox jumps over the lazy dog
  # blahblah
  master_collections = ["resources","master","system","slaves","frameworks","messages","evqueue","registrar"]

The following and any other attempt to add more chars to the comments, don't work:

# Telegraf plugin for gathering metrics from N Mesos masters
[[inputs.mesos]]
  # Timeout, in ms.
  timeout = 100
  # The quick brown fox jumps over the lazy dog
  # blahblaha
  masters = ["localhost:5050"]
  # The quick brown fox jumps over the lazy dog
  # blahblah
  master_collections = ["resources","master","system","slaves","frameworks","messages","evqueue","registrar"]

So still feels like something wrong in the parser...although I'm not 100% what's the combination that triggers it.

sparrc · 2016-02-16T22:56:16Z

that's very odd, I wish I could get rid of the naoina toml parser but unfortunately I haven't been able to find anything that has the same level of functionality :(

tripledes · 2016-02-16T23:00:41Z

@sparrc so if you agree, I'll shorten the comments and push the change.

Regarding the TOML parser, didn't you guys wanted to fork it? I kind of remember some comments in the influxdb project. I understand your point, but the naoina toml project seems rather dead...it hadn't got any activity for some months.

sparrc · 2016-02-17T01:01:34Z

yes please push the change to shorten comments.....I'm not really following the status of our config library, but it certainly seems like naoina is not maintaining the library very actively.

tripledes · 2016-02-17T01:14:37Z

I know this makes no sense, but just moving all mesos settings to memcache plugin section and removing the mesos section, makes the parser to crash in the same fashion....

Ok then, I'll push right away.

sparrc · 2016-02-17T01:33:24Z

🙈 🔫

sparrc · 2016-02-17T01:43:59Z

thanks @tripledes, I'll try to get this merged tomorrow

sparrc · 2016-02-18T00:38:14Z

internal/config/testdata/telegraf-agent.toml

@@ -184,6 +184,18 @@
  # If no servers are specified, then localhost is used as the host.
  servers = ["localhost"]

+# Telegraf plugin for gathering metrics from N Mesos masters
+[[inputs.mesos]]


can you make sure match the new SampleConfig?

sparrc · 2016-02-18T00:38:52Z

thanks @tripledes, just a couple places to make the new SampleConfig consistent

The plugin is able to query a Mesos master and push the metrics, a blacklist can be configured and a timeout, it's still not used. Added unit test, might be a good idea to have system test using docker.

The plugin will iterate over the Servers slice and create a goroutine for each of them.

* Now the user selects what to push instead of what not * Required to check and improve tests * Missing checks in the code when MetricsCol is empty

* Defined global var for holding default metric groups * Refactor removeGroup() to work with the whitelist * Refactor TestRemoveGroup()

* Use timeout as parameter in the http request * A bit of cleanup * More tests

* Fixed import for JSONFlattener{} it's now in parsers, broke after rebasing.

* This should help backwards compatibility when adding more features or supported Mesos components

* Use it as a paramater for the closure

And on the test configuration file

* It was still using the previous config name

sparrc · 2016-02-18T17:01:39Z

merged, thanks @tripledes

asdfsx · 2016-02-22T11:35:05Z

excellent job!!! @tripledes

closes influxdata#682

sparrc reviewed Feb 11, 2016
View reviewed changes

plugins/inputs/mesos/mesos_test.go

}

}

func TestSampleConfig(t *testing.T) {

Copy link

Contributor

sparrc Feb 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need to test this

sparrc added the Needs Work label Feb 15, 2016

sparrc removed the Needs Work label Feb 16, 2016

sparrc mentioned this pull request Feb 16, 2016

Telegraf 0.10.2 & InfluxDB 0.10.0 Field overflow #703

Closed

sparrc reviewed Feb 18, 2016
View reviewed changes

Sergio Jimenez added 2 commits February 18, 2016 09:00

plugin(mesos): Initial commit

05170d7

The plugin is able to query a Mesos master and push the metrics, a blacklist can be configured and a timeout, it's still not used. Added unit test, might be a good idea to have system test using docker.

Indentation for sample config string

59e0e49

Sergio Jimenez added 17 commits February 18, 2016 09:00

Don't add port to tags just the host

07502c9

plugin(mesos): Added goroutines.

1d50d62

The plugin will iterate over the Servers slice and create a goroutine for each of them.

plugin(mesos): Reversed removeGroup()

52b329b

* Now the user selects what to push instead of what not * Required to check and improve tests * Missing checks in the code when MetricsCol is empty

fix(indent): For configuration sample

4c1e817

feat(whitelist): Converted black to whitelist

9770802

* Defined global var for holding default metric groups * Refactor removeGroup() to work with the whitelist * Refactor TestRemoveGroup()

feat(timeout): Use timeout setting

babecb6

* Use timeout as parameter in the http request * A bit of cleanup * More tests

fix(import): Json parser lives outside internal

7170280

* Fixed import for JSONFlattener{} it's now in parsers, broke after rebasing.

Comments and cleanup

8c6a660

test(unit): Test for whitelisted metrics

7d10986

refactor(naming): For master specific settings

3cc2cda

* This should help backwards compatibility when adding more features or supported Mesos components

fix(vet): Range var used by goroutine

3573d93

* Use it as a paramater for the closure

List mesos in main README

38ac9d2

And on the test configuration file

fix(mesos): TOML annotation

29c671c

* It was still using the previous config name

test(unit): Removed useless tests

c8365b3

doc(readme): Added README.md.

e5f3acd

fix(sample): Made TOML parser happy again

1f77b75

fix(config): Made sample config consistent.

92e0fa2

sparrc closed this in 963c51f Feb 18, 2016

sparrc mentioned this pull request Feb 19, 2016

mesos input plugin #583

Closed

geodimm pushed a commit to miketonks/telegraf that referenced this pull request Mar 10, 2016

fix(config): Made sample config consistent.

f03a3e7

closes influxdata#682

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial support for Mesos master #682

Initial support for Mesos master #682

tripledes commented Feb 11, 2016

tripledes commented Feb 11, 2016

sparrc Feb 11, 2016

sparrc commented Feb 11, 2016

tripledes commented Feb 11, 2016

tripledes commented Feb 11, 2016

sparrc commented Feb 12, 2016

tripledes commented Feb 16, 2016

sparrc commented Feb 16, 2016

sparrc commented Feb 16, 2016

sparrc commented Feb 16, 2016

tripledes commented Feb 16, 2016

tripledes commented Feb 16, 2016

tripledes commented Feb 16, 2016

sparrc commented Feb 16, 2016

tripledes commented Feb 16, 2016

sparrc commented Feb 16, 2016

tripledes commented Feb 16, 2016

sparrc commented Feb 17, 2016

tripledes commented Feb 17, 2016

sparrc commented Feb 17, 2016

sparrc commented Feb 17, 2016

sparrc Feb 18, 2016

sparrc commented Feb 18, 2016

sparrc commented Feb 18, 2016

asdfsx commented Feb 22, 2016

Initial support for Mesos master #682

Initial support for Mesos master #682

Conversation

tripledes commented Feb 11, 2016

tripledes commented Feb 11, 2016

sparrc Feb 11, 2016

Choose a reason for hiding this comment

sparrc commented Feb 11, 2016

tripledes commented Feb 11, 2016

tripledes commented Feb 11, 2016

sparrc commented Feb 12, 2016

tripledes commented Feb 16, 2016

sparrc commented Feb 16, 2016

sparrc commented Feb 16, 2016

sparrc commented Feb 16, 2016

tripledes commented Feb 16, 2016

tripledes commented Feb 16, 2016

tripledes commented Feb 16, 2016

sparrc commented Feb 16, 2016

tripledes commented Feb 16, 2016

sparrc commented Feb 16, 2016

tripledes commented Feb 16, 2016

sparrc commented Feb 17, 2016

tripledes commented Feb 17, 2016

sparrc commented Feb 17, 2016

sparrc commented Feb 17, 2016

sparrc Feb 18, 2016

Choose a reason for hiding this comment

sparrc commented Feb 18, 2016

sparrc commented Feb 18, 2016

asdfsx commented Feb 22, 2016