wido commented on PR #13985: URL: https://github.com/apache/cloudstack/pull/13985#issuecomment-5541585561
> Thanks @wido, I think this is going in a much better direction. (btw, 53 lines only, I really gave my best) > > I like the change to `telemetry.*`, having one fixed reporting endpoint, moving the installation ID logic into a separate utility, and especially the tests around the actual JSON payload. This makes it much easier to understand exactly what is being sent and also makes future changes to the format more obvious. > > I don't want to repeat my previous very long review here :) but I also don't want it to look like the remaining points are no longer important. Since this is still WIP, I think there are a few important things we should keep in mind before this gets anywhere close to merge-ready: > > 1. **Installation ID** > I am OK with keeping the current derived ID approach for now, but I don't think we should say that it is guaranteed to be unique for every installation. Two installations could in theory have the same initial version and timestamp, and cloning a CloudStack DB would also produce the same ID. > I think the important thing here is to clearly define what we consider an "installation" and what we expect to happen with cloned/restored/reinstalled databases. > > 2. **Making sure reports are genuine** > The collector still has no way to know if a report really came from CloudStack. Anybody can read the source code, generate a random ID and send valid-looking JSON directly to the endpoint. > Apart from possible DoS/storage abuse, this also means somebody could pollute the statistics with fake installations. If we later use these statistics to make decisions about features, hypervisors, storage support, deprecations, etc., we need to have reasonable confidence that the data is real. > I think this is one of the main architectural points still to solve. > > 3. **Actual report schema validation** > The new client-side payload tests are very useful. > However, as far as I can see, the collector itself still validates only that the request is valid/sane JSON: depth, number/length of keys, value types, etc. It does not actually validate that the JSON matches the expected CloudStack telemetry structure. > For example, an unrelated JSON object with valid types would still pass the current validation. > I think we should have an explicit schema version in the report and validate the received payload against the expected schema. > > 4. **Current vs historical VM statistics** > One thing I think is especially important here is `vm_instance`. > CloudStack normally keeps VM rows in the database after the VM has been removed, so simply reading all `VMInstanceVO` objects can give us statistics about VMs which existed during the lifetime of the cloud, rather than the current size/state of the cloud. > Both numbers can actually be useful, but they mean very different things. I think we should explicitly separate current/non-removed VMs from historical/lifetime numbers. > > 5. **Privacy on the receiver side** > I don't have a problem with the actual report payload from a privacy point of view. The information being sent is aggregated and does not directly identify the operator. > My remaining concern is the HTTP side. The receiving infrastructure will see the source IP, and if normal HTTP/proxy access logging is enabled we can end up with a persistent installation ID associated with the public IP of the operator. > That could potentially identify the company/provider even though there is nothing identifying them inside the JSON itself. > So I think we need to define how source IPs, proxy headers, HTTP logs and retention are handled before the real collector is put into production. This probably also needs to be checked with ASF privacy/infra rather than only decided inside the CloudStack PMC. > > > There are still some other points from the previous review: only one MS should probably send the report in an HA setup, doing the aggregation in SQL instead of loading all objects into Java, active/stale installation tracking, actual volume statistics vs configured disk offerings, collector storage, and making the opt-in/report preview visible to operators. > > I don't think all of those need to be solved in the same commit though. They can be handled step by step while this is still WIP. > > Overall, definitely +1 from me on continuing with this and thx again for such a great initiative! The latest changes already address some of the previous concerns nicely. I would mainly make sure we agree on the points above before we consider the design stable and start moving towards merge-ready. Thanks for the review! Addressed in the latest push: 1. The chance of a collision is practically zero (same version + same creation second), and a cloned DB deriving the same ID is intended, it's the same cloud. Documented this in the README. 2. Agreed, but we can't prevent this: the source is open, anyone can craft a valid report. Schema validation and rate limiting bound the abuse; the statistics are best-effort by nature. 3. Done: the report now carries a schema_version and the collector validates the payload against the exact schema for that version, unexpected keys, missing sections or wrong value types are rejected. 4. Good point, done: instances is now split into current (non-removed, including Destroyed) and lifetime (all rows ever, with total/removed counts), using searchIncludingRemoved(). 5. Agreed. We have to write a privacy statement on this. We will receive the IP submitting the report. We don't see any problems with this at the Ceph project. We should however never store which IP submitted which report. Timestamps could be matched, that is true. We should try to log as minimal as possible. ASF infra will not host this for us btw. The directory has also been renamed from reporter/ to telemetry/ to match the feature name. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
