kubernetes-retired / spartakus

[EOL] Anonymous Usage Collector
Apache License 2.0
74 stars 30 forks source link

Add extensions reporting #19

Closed squat closed 7 years ago

squat commented 7 years ago

This pull request adds extensions reporting capabilities to the volunteer and collector. This addresses #10 by giving users the option to report metrics from a JSON file that can either be static or dynamically generated by a sidecar running alongside the volunteer.

Reports can be voluntarily extended to include additional information by running the volunteer with the --extensions flag. This flag should be set to the path of a file of arbitrary JSON key-value pairs you would like to report, e.g.:

spartakus volunteer --cluster-id=$(uuidgen) --extensions=/path/to/my/extensions.json

Where extensions.json could be:

{
  "hello": "world",
  "foo": "bar"
}

With this configuration, the volunteer will generate a report that looks like:

{
    "version": "v1.0.0",
    "timestamp": "867530909031976",
    "clusterID": "2f9c93d3-156c-47aa-8802-578ffca9b50e",
    "masterVersion": "v1.3.5",
    "extensions": [
      {
        "name": "hello",
        "value": "world"
      },
      {
        "name": "foo",
        "value": "bar"
      }
    ]
}

/cc @philips @sym3tri @thockin

Fixes #10

squat commented 7 years ago

ping @philips @thockin @sym3tri

sym3tri commented 7 years ago

Otherwise everything else here looks very reasonable and minimally invasive.

squat commented 7 years ago

@sym3tri I modified the volunteer code to not fail even if extensions listing fails. I also modified the tests to check for this case.

sym3tri commented 7 years ago

Looks good to me

thockin commented 7 years ago

ping me when ready for re-review

thockin commented 7 years ago

That was what I was thinking, just make sure to a) write to .foo.json and ignore files with a leading . when walking the directory. Or something like that.

Ping me when ready

On Mon, Mar 6, 2017 at 5:45 PM, Lucas Servén notifications@github.com wrote:

@squat commented on this pull request.

In cmd/spartakus/volunteer.go https://github.com/kubernetes-incubator/spartakus/pull/19#discussion_r104571334 :

@@ -42,6 +43,7 @@ func (_ volunteerSubProgram) AddFlags(fs *pflag.FlagSet) { fs.StringVar(&volunteerConfig.database, "database", "https://spartakus.k8s.io", "Send reports to this database; use --print-databases for a list of options") fs.BoolVar(&volunteerConfig.printDatabases, "print-databases", false, "Print database options and exit")

  • fs.StringVar(&volunteerConfig.extensionsPath, "extensions", "", "Path to a file of additional metrics to report; leave unset to report no additional metrics")

Hmm you bring up two good points.

  1. dealing with race condition between extension writers and spartakus
  2. mitigating race conditions between multiple extensions writers

The temp file + rename solution solves the race condition for a single writer + spartakus but still allows multiple writers to overwrite eachother. Regarding 2: I had not considered this use case. I spoke with @sym3tri https://github.com/sym3tri and we propose specifying an extensions directory instead of a path to a file. Spartakus will attempt to parse extensions from all of the files in that dir and merge them best effort. This allows different writers to write to separate files and not step on each other. Thoughts?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kubernetes-incubator/spartakus/pull/19#discussion_r104571334, or mute the thread https://github.com/notifications/unsubscribe-auth/AFVgVDi4YcflRCQUUECl237V-HsqpLcKks5rjLa5gaJpZM4L6Cr4 .

squat commented 7 years ago

@thockin thanks for the constructive feedback. I modified the extensionsLister to accept either a file or a directory; this way users will be able to write to multiple extensions files without the writers stomping on each other. I also spec'ed the extensions name key. PTAL

thanks

thockin commented 7 years ago

LGTM

thockin commented 7 years ago

I have added these fields to my bigquery instance schema