exported type MetricSet should have comment or be unexported

don't use an underscore in package name

don't use an underscore in package name

Indentation? And/or move this to a file?

I will use the file that I already created for the system tests ...

net.JoinHostPort()?

Return error also if jobsData.Jobs is nil?

I would say in case it's empty no error should be returned.

We could print the error instead, in case it is caused by other reason.

done

Could we use this file also for the integration test?

done

Are we going to collect more metrics?

Yes. This PR mainly creates the foundation.

Spelling: "none master" should be "non-master"

Also, "index summary stats" should probably be "machine learning job stats".

Both fixed.

When X-Pack isn't installed in ES, this if condition becomes true because a 400 error code is returned by the call to the /_xpack/ml/anomaly_detectors/_all/_stats API. This then results in error documents being indexed in the metricbeat index that look like this:

{
  "@timestamp":"2018-07-05T16:35:40.258Z",
  "metricset":{
    "rtt":6264,
    "namespace":"elasticsearch.ml.job",
    "name":"ml_job",
    "module":"elasticsearch",
    "host":"localhost:9200"
  },
  "error":{
    "message":"HTTP error 400 in ml_job: 400 Bad Request"
  },
  "beat":{
    "version":"7.0.0-alpha1",
    "name":"Shaunaks-MBP-2",
    "hostname":"Shaunaks-MBP-2"
  },
  "host":{
    "name":"Shaunaks-MBP-2"
  }
}

Just want to confirm that this is the behavior we want? Another option could be to log an error (but not index anything) if this API returns 400?

[EDIT] Obviously, whatever approach we take here should be consistent with all X-Pack APIs/metricsets.

Alternatively, could we somehow make use of the xpack config setting here?

If someone enables the ml_job metricset and X-Pack is not enabled I think an error document should be returned. What we probably should improve is the error message above and have something inside that it is not enabled.

I think our xpack config is not related to this.

Might be worth including invalid_date_count as well. From https://www.elastic.co/guide/en/elasticsearch/reference/6.2/ml-jobstats.html#ml-datacounts:

invalid_date_count
(long) The number of records with either a missing date field or a date that could not be parsed.

Added. In general I think we will have to add more fields but I also would like to involve the ML team here in a follow up. I don't think we should send all the stats but only the ones that are most relevant which is a tricky question to answer.

Should this say ml_job instead of ml?

changed

Nit: Name this enableTrialLicense just to be more specific about what it actually does?

It seems like the return value is being ignored anyway so my following comment my be moot:

Looking at other metricsets in the elasticsearch module, it looks like their eventsMapping functions return error instead of []error. Further, to build this error return object they use var errs multierror.Errors to collect multiple errors and then return errs.Err() at the end of the function. For consistency maybe the eventsMapping function here should do the same?

Yes, need to change it. This changed recently with the schema change from Jaime. Will adapt it accordingly.

This is more of question for my learning, not a review suggestion: why is bytes.NewReader more appropriate here than strings.NewReader?

The main difference is that one takes a []byte array and the other on a string as param. ReadFile above returns a []byte array and previously it was a string (mlJob var).