{"thread":{"id":"48670","subject":"[RFC PATCH v1] telemetry design overview (part 1)","startedAt":"2018-06-07T14:53:30Z","lastAt":"2018-06-12T16:04:44Z","messageCount":27,"participants":["git@jeffhostetler.com","Johannes Sixt","Jeff King","Ævar Arnfjörð Bjarmason","Duy Nguyen","Thomas Braun","Johannes Schindelin","brian m. carlson","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"349641","messageId":"20180607145313.25015-1-git@jeffhostetler.com","threadId":"48670","inReplyTo":null,"subject":"[RFC PATCH v1] telemetry design overview (part 1)","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2018-06-07T14:53:12Z","receivedAt":"2018-06-07T14:53:30Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nI've been working to add code to Git to optionally collect telemetry data.\nThe goal is to be able to collect performance data from Git commands and\nallow it to be aggregated over a user community to find \"slow commands\".\n\nI'm going to break this up into several parts rather than sending one large\npatch series. I think it is easier to review in pieces and in stages.\n\nPart 1 contains the overall design documentation.\nPart 2 will contain the basic telemetry event mechanism and the cmd_start\nand cmd_exit events.\n\nI'll post part 2 shortly -- as soon as I can convert my tests from python\nto perl. :-)\n\nJeff Hostetler (1):\n telemetry: design documenation\n\n Documentation/technical/telemetry.txt | 475 ++++++++++++++++++++++++++++++++++\n 1 file changed, 475 insertions(+)\n create mode 100644 Documentation/technical/telemetry.txt\n\n-- \n2.9.3\n\n"},{"id":"349642","messageId":"20180607145313.25015-2-git@jeffhostetler.com","threadId":"48670","inReplyTo":"20180607145313.25015-1-git@jeffhostetler.com","subject":"[RFC PATCH v1] telemetry: design documenation","fromName":"","fromEmail":"git@jeffhostetler.com","sentAt":"2018-06-07T14:53:13Z","receivedAt":"2018-06-07T14:53:34Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nCreate design documentation to describe the telemetry feature.\n\nSigned-off-by: Jeff Hostetler \n---\n Documentation/technical/telemetry.txt | 475 ++++++++++++++++++++++++++++++++++\n 1 file changed, 475 insertions(+)\n create mode 100644 Documentation/technical/telemetry.txt\n\ndiff --git a/Documentation/technical/telemetry.txt b/Documentation/technical/telemetry.txt\nnew file mode 100644\nindex 0000000..0a708ad\n--- /dev/null\n+++ b/Documentation/technical/telemetry.txt\n@@ -0,0 +1,475 @@\n+Telemetry Design Notes\n+======================\n+\n+The telemetry feature allows Git to generate structured telemetry data\n+for executed commands. Data includes command line arguments, execution\n+times, error codes and messages, and information about child processes.\n+\n+Structued data is produced in a JSON-like format. (See the UTF-8 related\n+\"limitations\" described in json-writer.h)\n+\n+Telemetry data can be written to a local file or sent to a dynamically\n+loaded shared library via a plugin API.\n+\n+The telemetry feature is similar to the existing trace API (defined in\n+Documentation/technical/api-trace.txt). Telemetry events are generated\n+thoughout the life of a Git command just like trace messages. But where\n+as trace messages are essentially developer debug messages, telemetry\n+events are intended for logging and automated analysis.\n+\n+The goal of the telemetry feature is to be able to gather usage data across\n+a group of production users to identify real-world performance problems in\n+production. Additionally, it might help identify common user errors and\n+guide future user training.\n+\n+By default, telemetry is disabled. Telemetry is controlled using config\n+settings (see \"telemetry.*\" in Documentation/config.txt).\n+\n+\n+Telemetry Events\n+================\n+\n+Telemetry data is generated as a series of events. Each event is written\n+as a self-describing JSON object.\n+\n+Events: cmd_start and cmd_exit\n+------------------------------\n+\n+The `cmd_start` event is emitted the very beginning of the git.exe process\n+in cmd_main() and `cmd_exit` event is emitted at the end of the process in\n+the atexit cleanup routine.\n+\n+For example, running \"git version\" produces:\n+\n+{\n+ \"event_name\": \"cmd_start\",\n+ \"argv\": [\n+ \"C:\\\\work\\\\gfw\\\\git.exe\",\n+ \"version\"\n+ ],\n+ \"clock\": 1525978509976086000,\n+ \"pid\": 25460,\n+ \"git_version\": \"2.17.0.windows.1\",\n+ \"telemetry_version\": \"1\",\n+ \"session_id\": \"1525978509976086000-25460\"\n+}\n+{\n+ \"event_name\": \"cmd_exit\",\n+ \"argv\": [\n+ \"C:\\\\work\\\\gfw\\\\git.exe\",\n+ \"version\"\n+ ],\n+ \"clock\": 1525978509980903391,\n+ \"pid\": 25460,\n+ \"git_version\": \"2.17.0.windows.1\",\n+ \"telemetry_version\": \"1\",\n+ \"session_id\": \"1525978509976086000-25460\",\n+ \"is_interactive\": false,\n+ \"exit_code\": 0,\n+ \"elapsed_time_core\": 0.004814,\n+ \"elapsed_time_total\": 0.004817,\n+ \"builtin\": {\n+ \"name\": \"version\"\n+ }\n+}\n+\n+Fields common to all events:\n+ * `event_name` is the name of the event.\n+ * `argv` is the array of command line arguments.\n+ * `clock` is the time of the event in nanoseconds since the epoch.\n+ * `pid` is the process id.\n+ * `git_version` is the git version string.\n+ * `telemetry_version` is the version of the telemetry format.\n+ * `session_id` is described in a later section.\n+\n+Additional fields in cmd_exit:\n+ * `is_interactive` is true if git.exe spawned an interactive child process,\n+ such as a pager, editor, prompt, or gui tool.\n+ * `exit_code` is the value passed to exit() from main().\n+ * `error_message` (not shown) is the array of error messages.\n+ * `elapsed-core-time` measures the time in seconds until exit() was called.\n+ * `elapsed-total-time` measures the time until the atexit() routine starts\n+ (which will include time spend in other atexit() routines cleaning up\n+ child processes and etc.).\n+ * `alias` (not shown) the updated argv after alias expansion.\n+ * `builtin.name` is the canonical command name (from the cmd_struct[]\n+ table) of a builtin command.\n+ * `builtin.mode` (not shown) is shown for some commands that have different\n+ major modes and performance times. For example, checkout can switch\n+ branches or repair a single file.\n+ * `child_summary` (not shown) is described in a later section.\n+ * `timers` (not shown) is described in a later section.\n+ * `aux` (not shown) is described in a later section.\n+\n+\n+Events: child_start and child_exit\n+----------------------------------\n+\n+The child-start event is emitted just before a child process is started.\n+It includes a unique child-id and the child's command line arguments.\n+\n+The child-exit event is emitted after a child process exits and has\n+been reaped. This event extends the start event with the child's exit\n+status and elapsed time.\n+\n+For example, during a \"git fetch origin\", git.exe runs gc in the background\n+and these events are emitted by the fetch process before and after the\n+child gc process:\n+\n+{\n+ \"event_name\": \"child_start\",\n+ \"argv\": [\n+ \"C:\\\\work\\\\gfw\\\\git.exe\",\n+ \"fetch\",\n+ \"origin\"\n+ ],\n+ \"clock\": 1525979478738132887,\n+ \"pid\": 18332,\n+ \"git_version\": \"2.17.0.windows.1\",\n+ \"telemetry_version\": \"1\",\n+ \"session_id\": \"1525979470792747000-18332\",\n+ \"child_detail\": {\n+ \"number\": 3,\n+ \"class\": \"gc\",\n+ \"argv\": [\n+ \"gc\",\n+ \"--auto\"\n+ ]\n+ }\n+}\n+{\n+ \"event_name\": \"child_exit\",\n+ \"argv\": [\n+ \"C:\\\\work\\\\gfw\\\\git.exe\",\n+ \"fetch\",\n+ \"origin\"\n+ ],\n+ \"clock\": 1525979479024707085,\n+ \"pid\": 18332,\n+ \"git_version\": \"2.17.0.windows.1\",\n+ \"telemetry_version\": \"1\",\n+ \"session_id\": \"1525979470792747000-18332\",\n+ \"child_detail\": {\n+ \"number\": 3,\n+ \"class\": \"gc\",\n+ \"argv\": [\n+ \"gc\",\n+ \"--auto\"\n+ ],\n+ \"pid\": 19608,\n+ \"exit_code\": 0,\n+ \"elapsed_time\": 0.286574\n+ }\n+}\n+\n+The common fields (`event_name` through `session_id`) are the same as\n+in the `cmd_start` and `cmd_exit` events and refer to the parent process.\n+\n+The `child_detail` structure describes the child process:\n+ * `number` is a simple counter incremented for each child event.\n+ * `class` is a rough characterization of the type of child process. Child\n+ class is described in a later section.\n+ * `argv` is the child's command line.\n+ * `pid` is the child's process id.\n+ * `exit_code` is the exit code of the child process.\n+ * `elapsed_time` measures the time in seconds observed by the parent process\n+ between the child_start and child_exit events. This will be greater\n+ than the elapsed time that the child internally observes because of\n+ process startup and shutdown overhead. For synchronous child processes,\n+ this is the time that the parent spent waiting for the child.\n+\n+\n+Event: perf\n+-----------\n+\n+Perf events are a debugging aid to report on suspected hot spots in the\n+code and collect data from production users. This is intended to be a\n+generic message with context-specific data. New messages may be added\n+in the future as the need arises to help with debugging.\n+\n+Perf events are organized by category, much like the various GIT_TRACE_*\n+environment variables. The \"telemetry.perf\" config setting can be set to\n+true or to a string of the perf categories that should be enabled.\n+\n+Currently, the categories \"index\" and \"status\" are defined. Others may\n+be added later.\n+\n+For example, could be used to instrument read_index_from():\n+\n+{\n+ \"event_name\": \"perf\",\n+ \"argv\": [\n+ \"C:\\\\work\\\\gfw\\\\git.exe\",\n+ \"fetch\",\n+ \"origin\"\n+ ],\n+ \"clock\": 1525979478735438090,\n+ \"pid\": 18332,\n+ \"git_version\": \"2.17.0.windows.1\",\n+ \"telemetry_version\": \"1\",\n+ \"session_id\": \"1525979470792747000-18332\",\n+ \"category\": \"index\",\n+ \"label\": \"read_index_from\",\n+ \"elapsed_time\": 0.001536,\n+ \"data\": {\n+ \"path\": \".git/index\",\n+ \"cache_nr\": 3311\n+ }\n+}\n+\n+The common fields (`event_name` through `session_id`) are the same as\n+in the `cmd_start` and `cmd_exit` events.\n+\n+All `perf` events also have:\n+ * `category` is descriptive category and used like different GIT_TRACE_*\n+ variables.\n+ * `label` is the name of a function or region of interest.\n+ * `elapsed_time` measures the time in seconds spent in the function or\n+ region.\n+ * `data` is an optional structure of context-specific (debug) data.\n+\n+\n+More Details for Event Fields\n+=============================\n+\n+Field: session_id\n+-----------------\n+\n+A session_id (SID) is a cheap, unique-enough string to associate all of\n+the events generated by a single process. They incorporate the inherited\n+SID from their parent process.\n+\n+SIDs should be considerd opaque data, but are constructed as:\n+\n+ []/-\n+\n+This scheme is used rather than a simple PID or {PPID, PID} because PIDs\n+are recycled by the OS (after sufficient time). Also, if telemetry data\n+is aggregated from multiple systems, PIDs are not sufficient.\n+\n+This also has the advantage of allowing telemetry analysis to associate\n+Git child processes with their Git parent process even if there are\n+intermediate shell processes.\n+\n+Note: we could use UUIDs or GUIDs for this, but that seemed overkill at\n+this point. It also required platform-specific code to generate which\n+muddied up the code.\n+\n+\n+Field: child_details.class\n+--------------------------\n+\n+enum telemetry_class contains a set of classification values. These attempt\n+to roughly classify a child process from the point of view of the parent\n+process.\n+ * unclass: unclassified\n+ * unclass-async: unclassified asynchronous child (see sub-process.c)\n+ * alias: an alias expansion using a child process\n+ * hook: a hook process that may do anything (including prompting, scanning,\n+ and network operations) and wildly affect command run times.\n+ * pager: a pager (indicating an interactive command)\n+ * editor: an editor (indicating an interactive command)\n+ * prompt: a prompt or credential or askpass process (also interactive)\n+ * network: a command that might do network operations\n+ * convert: an attribute filter process such as LFS or CRLF\n+ * tool: a tool, such as difftool or mergetool, that may be interactive\n+ * gc: an auto gc process\n+\n+struct child_process has been extended to have a telemetry_class field. Some\n+callers of start_command() and/or run_command() have been updated to suggest\n+a classification when appropriate. For example, child processes created by\n+launch_editor() are marked with TELEMETRY_CLASS__EDITOR.\n+\n+The primary intent is to identify which child processes are likely to block\n+on the user or network. For example, \"git commit\" and \"git commit -m \"\n+will have different performance characteristics because the former has to\n+launch an editor and wait for the user to compose a message. The former will\n+have a child event which child_detail.class=editor and its exit event will\n+have child_summary.editor.count=1 and child_summary.editor.elapsed_time=.\n+Analysis tools can choose to report average commit time for non-interactive\n+commands or subtract the editor elapsed time from the commit elapsed time.\n+\n+For example, fetch runs rev-list, ssh, index-pack, and maybe (auto) gc. The\n+ssh child is marked as TELEMETRY_CLASS__NETWORK and the gc child is marked\n+as TELEMETRY_CLASS__GC (since it is optional and possibly time consuming).\n+The others are left unclassified (TELEMETRY_CLASS__UNCLASS) since we don't\n+expect blocking operations.\n+\n+\n+Field: child_summary\n+--------------------\n+\n+The `child_summary` structure within the `cmd_exit` event summarizes the\n+child processes created by the parent process.\n+\n+For example, \"git fetch origin\" spawns 4 child processes:\n+\n+{\n+ \"event_name\": \"cmd_exit\",\n+ \"argv\": [\n+ \"C:\\\\work\\\\gfw\\\\git.exe\",\n+ \"fetch\",\n+ \"origin\"\n+ ],\n+ ...\n+ \"child_summary\": {\n+ \"unclass\": {\n+ \"count\": 2,\n+ \"elapsed_time\": 0.496387\n+ },\n+ \"network\": {\n+ \"count\": 1,\n+ \"elapsed_time\": 7.712466\n+ },\n+ \"gc\": {\n+ \"count\": 1,\n+ \"elapsed_time\": 0.286574\n+ }\n+ },\n+ \"exit_code\": 0,\n+ \"elapsed_time_core\": 8.232965,\n+ \"elapsed_time_total\": 8.232968,\n+ \"builtin\": {\n+ \"name\": \"fetch\"\n+ }\n+}\n+\n+Within each `child_summary[]` is a count of the number of child\n+processes and the cummulative elapsed time.\n+\n+Analysis tools interested in a net-elapsed-time of the parent process may\n+want to subtract the elapsed time of the child processes. This approach is\n+mostly valid, since most child processes are run synchronously. However,\n+some processes are run asynchronously, such as the pager and processes in\n+the unclass-async pool, so care should be taken.\n+\n+\n+Field: timers\n+-------------\n+\n+A \"telemetry timer\" is a stopwatch-like timer with a counter. It can be\n+used to time a specific region of code, such as an expensive computation\n+within the body of a larger loop. It defines a generic way to collect\n+perf data without causing an telemetry perf event to be fired on each\n+iteration. Instead, a timer is registered with the telemetry layer and\n+the data will be included in a \"timers\" sub-section in the `cmd_exit` event.\n+\n+For example, a timer was added to do_read_index() and do_write_index()\n+to measure the time spent reading and writing the index.\n+\n+{\n+ \"event_name\": \"cmd_exit\",\n+ \"argv\": [\n+ \"C:\\\\work\\\\gfw\\\\git.exe\",\n+ \"status\"\n+ ],\n+ ...\n+ \"timers\": {\n+ \"do_read_index\": {\n+ \"count\": 1,\n+ \"total\": 0.000740,\n+ \"min\": 0.000740,\n+ \"max\": 0.000740,\n+ \"avg\": 0.000740\n+ },\n+ \"do_write_index\": {\n+ \"count\": 1,\n+ \"total\": 0.004724,\n+ \"min\": 0.004724,\n+ \"max\": 0.004724,\n+ \"avg\": 0.004724\n+ }\n+ },\n+ \"exit_code\": 0,\n+ \"elapsed_time_core\": 0.049865,\n+ \"elapsed_time_total\": 0.049867,\n+ \"builtin\": {\n+ \"name\": \"status\"\n+ }\n+}\n+\n+The `timers` structure contains a named member for each defined timer.\n+Within each individual timer, we have:\n+ * `count` is the number of times it was started/stopped.\n+ * `total` is the total time the timer was running.\n+ * `min` is the shortest interval.\n+ * `max` is the longest interval.\n+ * `avg` is the average interval.\n+\n+\n+Field: aux\n+---------------\n+\n+The `aux` structure within the `cmd_exit` event contains additional\n+information about the process. This is intended as a generic container for\n+various fields, such as important config settings or repo data shape that\n+may affect performance or help identify the repository for aggregation\n+purposes.\n+\n+{\n+ \"event_name\": \"cmd_exit\",\n+ ...\n+ \"aux\": {\n+ \"remote_origin_url\": \"git@github.com:git/git.git\",\n+ \"index_count\": 3311,\n+ \"sparse_checkout_count\": 3\n+ },\n+ ...\n+}\n+\n+Other fields (and even sub-structures) can be added to this container\n+as needed.\n+\n+\n+Telemetry Destination\n+=====================\n+\n+Telemetry events are sent to a \"destination\". This can be a file or a\n+plugin. Telemetry is disabled if a destination is not set.\n+\n+telemetry.path\n+--------------\n+\n+If the config setting \"telemetry.path\" contains a pathname, telemetry\n+events will be appended to that file using the builtin destination\n+handler. (File rotation is beyond the scope of this design.)\n+\n+Events are written as a series of JSON records. When \"telemetry.pretty\"\n+is false, each event record will be written on one line.\n+\n+(All of the examples in this document were prepared with \"telemetry.pretty\"\n+set to true.)\n+\n+telemetry.plugin\n+----------------\n+\n+If the config setting \"telemetry.plugin\" contains the pathname to a shared\n+library, the library will be dynamically loaded during start up and events\n+will be sent to it using the plugin API.\n+\n+This plugin model allows an organization to define a custom or private\n+telemetry solution while using a stock version of Git.\n+\n+For example, on Windows, it allows telemetry events to go directly to the\n+kernel via the plugin using the high performance Event Tracing for Windows\n+(ETW) facility.\n+\n+The contrib/telemetry-plugin-examples directory contains two example\n+plugins:\n+ * A trivial log to stderr\n+ * A trivial ETW writer\n+\n+\n+GDPR and Privacy\n+================\n+\n+The telemetry feature can log possibly sensitive user information (such as\n+command line arguments, which may contain URLs, user names, and file names).\n+\n+The base telemetry feature can write telemetry data to a file on the system.\n+\n+The plugin facility can be used to publish the telemetry data to more general\n+destinations (such as ETW or the network).\n+\n+In both cases, it is up to the user or system administrator to decide what\n+is appropriate and sanitize the data accordingly before broadcasting it.\n-- \n2.9.3\n\n"},{"id":"349667","messageId":"c3ed8128-1184-8199-06e5-a4e96b2bc7c5@kdbg.org","threadId":"48670","inReplyTo":"20180607145313.25015-1-git@jeffhostetler.com","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-06-07T21:10:52Z","receivedAt":"2018-06-07T21:10:58Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 07.06.2018 um 16:53 schrieb git@jeffhostetler.com:\n> From: Jeff Hostetler \n> \n> I've been working to add code to Git to optionally collect telemetry data.\n> The goal is to be able to collect performance data from Git commands and\n> allow it to be aggregated over a user community to find \"slow commands\".\n\nSeriously? \"add code to collect telemetry data\" said by somebody whose \nemail address ends with @microsoft.com is very irritating. I really \ndon't want to have yet another switch that I must check after every \nupdate that it is still off.\n\n-- Hannes\n"},{"id":"349685","messageId":"20180608090758.GA15112@sigill.intra.peff.net","threadId":"48670","inReplyTo":"c3ed8128-1184-8199-06e5-a4e96b2bc7c5@kdbg.org","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-08T09:07:58Z","receivedAt":"2018-06-08T09:08:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 07, 2018 at 11:10:52PM +0200, Johannes Sixt wrote:\n\n> Am 07.06.2018 um 16:53 schrieb git@jeffhostetler.com:\n> > From: Jeff Hostetler \n> > \n> > I've been working to add code to Git to optionally collect telemetry data.\n> > The goal is to be able to collect performance data from Git commands and\n> > allow it to be aggregated over a user community to find \"slow commands\".\n> \n> Seriously? \"add code to collect telemetry data\" said by somebody whose email\n> address ends with @microsoft.com is very irritating. I really don't want to\n> have yet another switch that I must check after every update that it is\n> still off.\n\nIf you look at the design document, it's off by default and would write\nto a file on the filesystem. That doesn't seem all that different from\nGIT_TRACE.\n\n-Peff\n"},{"id":"349686","messageId":"871sdhzkd2.fsf@evledraar.gmail.com","threadId":"48670","inReplyTo":"c3ed8128-1184-8199-06e5-a4e96b2bc7c5@kdbg.org","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T09:40:57Z","receivedAt":"2018-06-08T09:41:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jun 07 2018, Johannes Sixt wrote:\n\n> Am 07.06.2018 um 16:53 schrieb git@jeffhostetler.com:\n>> From: Jeff Hostetler \n>>\n>> I've been working to add code to Git to optionally collect telemetry data.\n>> The goal is to be able to collect performance data from Git commands and\n>> allow it to be aggregated over a user community to find \"slow commands\".\n>\n> Seriously? \"add code to collect telemetry data\" said by somebody whose\n> email address ends with @microsoft.com is very irritating. I really\n> don't want to have yet another switch that I must check after every\n> update that it is still off.\n\nTo elaborate a bit on Jeff's reply (since this was discussed in more\ndetail at Git Merge this year), the point of this feature is not to ship\ngit.git with some default telemetry, but so that in-house users of git\nlike Microsoft, Dropbox, Booking.com etc. can build & configure their\ninternal versions of git to turn on telemetry for their own users.\n\nThere's numerous in-house monkeypatches to git on various sites (at\nleast Microsoft & Dropbox reported having internal patches already).\nSomething like this facility would allow us to agree on some\nimplementation that could be shipped by default (but would be off by\ndefault), those of us who'd make use of this feature already have \"root\"\non those user's machines, and control what git binary they use etc,\ntheir /etc/gitconfig and so on.\n"},{"id":"349688","messageId":"87zi05y1ug.fsf@evledraar.gmail.com","threadId":"48670","inReplyTo":"20180607145313.25015-2-git@jeffhostetler.com","subject":"Re: [RFC PATCH v1] telemetry: design documenation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T11:06:15Z","receivedAt":"2018-06-08T11:06:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jun 07 2018, git@jeffhostetler.com wrote:\n\n> From: Jeff Hostetler \n>\n> Create design documentation to describe the telemetry feature.\n>\n> Signed-off-by: Jeff Hostetler \n> ---\n> Documentation/technical/telemetry.txt | 475 ++++++++++++++++++++++++++++++++++\n> 1 file changed, 475 insertions(+)\n> create mode 100644 Documentation/technical/telemetry.txt\n>\n> diff --git a/Documentation/technical/telemetry.txt b/Documentation/technical/telemetry.txt\n> new file mode 100644\n> index 0000000..0a708ad\n> --- /dev/null\n> +++ b/Documentation/technical/telemetry.txt\n> @@ -0,0 +1,475 @@\n> +Telemetry Design Notes\n> +======================\n> +\n> +The telemetry feature allows Git to generate structured telemetry data\n> +for executed commands. Data includes command line arguments, execution\n> +times, error codes and messages, and information about child processes.\n> +\n> +Structued data is produced in a JSON-like format. (See the UTF-8 related\n\ns/Structued/Structured/\n\n> +\"limitations\" described in json-writer.h)\n> +\n> +Telemetry data can be written to a local file or sent to a dynamically\n> +loaded shared library via a plugin API.\n> +\n> +The telemetry feature is similar to the existing trace API (defined in\n> +Documentation/technical/api-trace.txt). Telemetry events are generated\n> +thoughout the life of a Git command just like trace messages. But where\n\ns/thoughout/throughout/\n\n> +as trace messages are essentially developer debug messages, telemetry\n> +events are intended for logging and automated analysis.\n> +\n> +The goal of the telemetry feature is to be able to gather usage data across\n> +a group of production users to identify real-world performance problems in\n> +production. Additionally, it might help identify common user errors and\n> +guide future user training.\n> +\n> +By default, telemetry is disabled. Telemetry is controlled using config\n> +settings (see \"telemetry.*\" in Documentation/config.txt).\n> +\n> +\n> +Telemetry Events\n> +================\n> +\n> +Telemetry data is generated as a series of events. Each event is written\n> +as a self-describing JSON object.\n> +\n> +Events: cmd_start and cmd_exit\n> +------------------------------\n> +\n> +The `cmd_start` event is emitted the very beginning of the git.exe process\n> +in cmd_main() and `cmd_exit` event is emitted at the end of the process in\n> +the atexit cleanup routine.\n> +\n> +For example, running \"git version\" produces:\n> +\n> +{\n> + \"event_name\": \"cmd_start\",\n> + \"argv\": [\n> + \"C:\\\\work\\\\gfw\\\\git.exe\",\n> + \"version\"\n> + ],\n> + \"clock\": 1525978509976086000,\n> + \"pid\": 25460,\n> + \"git_version\": \"2.17.0.windows.1\",\n> + \"telemetry_version\": \"1\",\n> + \"session_id\": \"1525978509976086000-25460\"\n> +}\n> +{\n> + \"event_name\": \"cmd_exit\",\n> + \"argv\": [\n> + \"C:\\\\work\\\\gfw\\\\git.exe\",\n> + \"version\"\n> + ],\n> + \"clock\": 1525978509980903391,\n> + \"pid\": 25460,\n> + \"git_version\": \"2.17.0.windows.1\",\n> + \"telemetry_version\": \"1\",\n> + \"session_id\": \"1525978509976086000-25460\",\n> + \"is_interactive\": false,\n> + \"exit_code\": 0,\n> + \"elapsed_time_core\": 0.004814,\n> + \"elapsed_time_total\": 0.004817,\n> + \"builtin\": {\n> + \"name\": \"version\"\n> + }\n> +}\n> +\n> +Fields common to all events:\n> + * `event_name` is the name of the event.\n> + * `argv` is the array of command line arguments.\n> + * `clock` is the time of the event in nanoseconds since the epoch.\n> + * `pid` is the process id.\n> + * `git_version` is the git version string.\n> + * `telemetry_version` is the version of the telemetry format.\n> + * `session_id` is described in a later section.\n\nI wonder if we should add the path to git to these if RUNTIME_PREFIX is\ndefined. Would be useful to log experiments with different git versions,\nand as noted by the RUNTIME_PREFIX feature this information is not\nguaranteed to be something you can extract from argv.\n\n> +Additional fields in cmd_exit:\n> + * `is_interactive` is true if git.exe spawned an interactive child process,\n> + such as a pager, editor, prompt, or gui tool.\n> + * `exit_code` is the value passed to exit() from main().\n> + * `error_message` (not shown) is the array of error messages.\n> + * `elapsed-core-time` measures the time in seconds until exit() was called.\n> + * `elapsed-total-time` measures the time until the atexit() routine starts\n> + (which will include time spend in other atexit() routines cleaning up\n> + child processes and etc.).\n> + * `alias` (not shown) the updated argv after alias expansion.\n> + * `builtin.name` is the canonical command name (from the cmd_struct[]\n> + table) of a builtin command.\n> + * `builtin.mode` (not shown) is shown for some commands that have different\n> + major modes and performance times. For example, checkout can switch\n> + branches or repair a single file.\n> + * `child_summary` (not shown) is described in a later section.\n> + * `timers` (not shown) is described in a later section.\n> + * `aux` (not shown) is described in a later section.\n> +\n> +\n> +Events: child_start and child_exit\n> +----------------------------------\n> +\n> +The child-start event is emitted just before a child process is started.\n> +It includes a unique child-id and the child's command line arguments.\n> +\n> +The child-exit event is emitted after a child process exits and has\n> +been reaped. This event extends the start event with the child's exit\n> +status and elapsed time.\n> +\n> +For example, during a \"git fetch origin\", git.exe runs gc in the background\n> +and these events are emitted by the fetch process before and after the\n> +child gc process:\n> +\n> +{\n> + \"event_name\": \"child_start\",\n> + \"argv\": [\n> + \"C:\\\\work\\\\gfw\\\\git.exe\",\n> + \"fetch\",\n> + \"origin\"\n> + ],\n> + \"clock\": 1525979478738132887,\n> + \"pid\": 18332,\n> + \"git_version\": \"2.17.0.windows.1\",\n> + \"telemetry_version\": \"1\",\n> + \"session_id\": \"1525979470792747000-18332\",\n> + \"child_detail\": {\n> + \"number\": 3,\n> + \"class\": \"gc\",\n> + \"argv\": [\n> + \"gc\",\n> + \"--auto\"\n> + ]\n> + }\n> +}\n> +{\n> + \"event_name\": \"child_exit\",\n> + \"argv\": [\n> + \"C:\\\\work\\\\gfw\\\\git.exe\",\n> + \"fetch\",\n> + \"origin\"\n> + ],\n> + \"clock\": 1525979479024707085,\n> + \"pid\": 18332,\n> + \"git_version\": \"2.17.0.windows.1\",\n> + \"telemetry_version\": \"1\",\n> + \"session_id\": \"1525979470792747000-18332\",\n> + \"child_detail\": {\n> + \"number\": 3,\n> + \"class\": \"gc\",\n> + \"argv\": [\n> + \"gc\",\n> + \"--auto\"\n> + ],\n> + \"pid\": 19608,\n> + \"exit_code\": 0,\n> + \"elapsed_time\": 0.286574\n> + }\n> +}\n> +\n> +The common fields (`event_name` through `session_id`) are the same as\n> +in the `cmd_start` and `cmd_exit` events and refer to the parent process.\n> +\n> +The `child_detail` structure describes the child process:\n> + * `number` is a simple counter incremented for each child event.\n> + * `class` is a rough characterization of the type of child process. Child\n> + class is described in a later section.\n> + * `argv` is the child's command line.\n> + * `pid` is the child's process id.\n> + * `exit_code` is the exit code of the child process.\n> + * `elapsed_time` measures the time in seconds observed by the parent process\n> + between the child_start and child_exit events. This will be greater\n> + than the elapsed time that the child internally observes because of\n> + process startup and shutdown overhead. For synchronous child processes,\n> + this is the time that the parent spent waiting for the child.\n> +\n> +\n> +Event: perf\n> +-----------\n> +\n> +Perf events are a debugging aid to report on suspected hot spots in the\n> +code and collect data from production users. This is intended to be a\n> +generic message with context-specific data. New messages may be added\n> +in the future as the need arises to help with debugging.\n> +\n> +Perf events are organized by category, much like the various GIT_TRACE_*\n> +environment variables. The \"telemetry.perf\" config setting can be set to\n> +true or to a string of the perf categories that should be enabled.\n> +\n> +Currently, the categories \"index\" and \"status\" are defined. Others may\n> +be added later.\n> +\n> +For example, could be used to instrument read_index_from():\n> +\n> +{\n> + \"event_name\": \"perf\",\n> + \"argv\": [\n> + \"C:\\\\work\\\\gfw\\\\git.exe\",\n> + \"fetch\",\n> + \"origin\"\n> + ],\n> + \"clock\": 1525979478735438090,\n> + \"pid\": 18332,\n> + \"git_version\": \"2.17.0.windows.1\",\n> + \"telemetry_version\": \"1\",\n> + \"session_id\": \"1525979470792747000-18332\",\n> + \"category\": \"index\",\n> + \"label\": \"read_index_from\",\n> + \"elapsed_time\": 0.001536,\n> + \"data\": {\n> + \"path\": \".git/index\",\n> + \"cache_nr\": 3311\n> + }\n> +}\n> +\n> +The common fields (`event_name` through `session_id`) are the same as\n> +in the `cmd_start` and `cmd_exit` events.\n> +\n> +All `perf` events also have:\n> + * `category` is descriptive category and used like different GIT_TRACE_*\n> + variables.\n> + * `label` is the name of a function or region of interest.\n> + * `elapsed_time` measures the time in seconds spent in the function or\n> + region.\n> + * `data` is an optional structure of context-specific (debug) data.\n> +\n> +\n> +More Details for Event Fields\n> +=============================\n> +\n> +Field: session_id\n> +-----------------\n> +\n> +A session_id (SID) is a cheap, unique-enough string to associate all of\n> +the events generated by a single process. They incorporate the inherited\n> +SID from their parent process.\n> +\n> +SIDs should be considerd opaque data, but are constructed as:\n\ns/considerd/considered/\n\n> +\n> + []/-\n> +\n> +This scheme is used rather than a simple PID or {PPID, PID} because PIDs\n> +are recycled by the OS (after sufficient time). Also, if telemetry data\n> +is aggregated from multiple systems, PIDs are not sufficient.\n> +\n> +This also has the advantage of allowing telemetry analysis to associate\n> +Git child processes with their Git parent process even if there are\n> +intermediate shell processes.\n> +\n> +Note: we could use UUIDs or GUIDs for this, but that seemed overkill at\n> +this point. It also required platform-specific code to generate which\n> +muddied up the code.\n\nFully agreed, UUID would be overkill, and it's a very useful property to\nhave the timestamp in there, since this is very likely to end up in a\nstorage system where this is a primary key, and being able to just look\nat those and see from what time they without also looking at \"clock\" is\nuseful.\n\nJust as a bit of bikeshedding, I wonder if this would be better with the\n always at the start,\ni.e. --. Without a parent it would be\nTIME--PID but with a parent TIME-PARENT_SID-PID.\n\nThe reason I'm suggesting that that is that if I e.g. make a MySQL table\nto insert these records and use the SID as a primary key, the inserts\nare much more efficient if they're in sorted order and all sort after\nexisting data. That's not *guaranteed* with my proposed amendmend, but\nas a practical matter would be the case most of the time.\n\nThis is a property shared by a lot of DB storage formats, and it would\nbe useful to cater to it.\n\nThat's mostly true of -- too, but there's\ngoing to be cases (e.g. gc) where we're starting subprocesses for a long\ntime, so by inserting new entries starting with at the\nbeginning we're having to write behind in such a system.\n\nBut maybe you were aiming for the property of having all events invoked\nby a given git (and the children) really closely packed together in such\na system.\n\nI don't feel strongly about it, just something to consider.\n\n> +Field: child_details.class\n> +--------------------------\n> +\n> +enum telemetry_class contains a set of classification values. These attempt\n> +to roughly classify a child process from the point of view of the parent\n> +process.\n> + * unclass: unclassified\n> + * unclass-async: unclassified asynchronous child (see sub-process.c)\n> + * alias: an alias expansion using a child process\n> + * hook: a hook process that may do anything (including prompting, scanning,\n> + and network operations) and wildly affect command run times.\n> + * pager: a pager (indicating an interactive command)\n> + * editor: an editor (indicating an interactive command)\n> + * prompt: a prompt or credential or askpass process (also interactive)\n> + * network: a command that might do network operations\n> + * convert: an attribute filter process such as LFS or CRLF\n> + * tool: a tool, such as difftool or mergetool, that may be interactive\n> + * gc: an auto gc process\n> +\n> +struct child_process has been extended to have a telemetry_class field. Some\n> +callers of start_command() and/or run_command() have been updated to suggest\n> +a classification when appropriate. For example, child processes created by\n> +launch_editor() are marked with TELEMETRY_CLASS__EDITOR.\n> +\n> +The primary intent is to identify which child processes are likely to block\n> +on the user or network. For example, \"git commit\" and \"git commit -m \"\n> +will have different performance characteristics because the former has to\n> +launch an editor and wait for the user to compose a message. The former will\n> +have a child event which child_detail.class=editor and its exit event will\n> +have child_summary.editor.count=1 and child_summary.editor.elapsed_time=.\n> +Analysis tools can choose to report average commit time for non-interactive\n> +commands or subtract the editor elapsed time from the commit elapsed time.\n> +\n> +For example, fetch runs rev-list, ssh, index-pack, and maybe (auto) gc. The\n> +ssh child is marked as TELEMETRY_CLASS__NETWORK and the gc child is marked\n> +as TELEMETRY_CLASS__GC (since it is optional and possibly time consuming).\n> +The others are left unclassified (TELEMETRY_CLASS__UNCLASS) since we don't\n> +expect blocking operations.\n\nThis whole thing is very useful, and worth the small effort on our end\nto classify commands as \"might use network\" etc.\n\n> +Field: child_summary\n> +--------------------\n> +\n> +The `child_summary` structure within the `cmd_exit` event summarizes the\n> +child processes created by the parent process.\n> +\n> +For example, \"git fetch origin\" spawns 4 child processes:\n> +\n> +{\n> + \"event_name\": \"cmd_exit\",\n> + \"argv\": [\n> + \"C:\\\\work\\\\gfw\\\\git.exe\",\n> + \"fetch\",\n> + \"origin\"\n> + ],\n> + ...\n> + \"child_summary\": {\n> + \"unclass\": {\n> + \"count\": 2,\n> + \"elapsed_time\": 0.496387\n> + },\n> + \"network\": {\n> + \"count\": 1,\n> + \"elapsed_time\": 7.712466\n> + },\n> + \"gc\": {\n> + \"count\": 1,\n> + \"elapsed_time\": 0.286574\n> + }\n> + },\n\nIf we're going to have \"clock\" be the nanosecond epoch, let's not have\nFLOAT seconds here, but instead just make \"elapsed_time\" be the elapsed\ntime in nanoseconds.\n\nThat'll both simplify dealing with this manually, and e.g. the schema of\nany DB that this gets inserted into.\n\n> + \"exit_code\": 0,\n> + \"elapsed_time_core\": 8.232965,\n> + \"elapsed_time_total\": 8.232968,\n\nDitto the parent's elapsed time....\n\n> + \"builtin\": {\n> + \"name\": \"fetch\"\n> + }\n> +}\n> +\n> +Within each `child_summary[]` is a count of the number of child\n> +processes and the cummulative elapsed time.\n\ns/cummulative/cumulative/\n\n> +Analysis tools interested in a net-elapsed-time of the parent process may\n> +want to subtract the elapsed time of the child processes. This approach is\n> +mostly valid, since most child processes are run synchronously. However,\n> +some processes are run asynchronously, such as the pager and processes in\n> +the unclass-async pool, so care should be taken.\n\n... Exactly for this reason, now anyone wanting to do that math is\nhaving to deal with the minefield that is IEEE Floating-Point\nArithmetic.\n\n> +Field: timers\n> +-------------\n> +\n> +A \"telemetry timer\" is a stopwatch-like timer with a counter. It can be\n> +used to time a specific region of code, such as an expensive computation\n> +within the body of a larger loop. It defines a generic way to collect\n> +perf data without causing an telemetry perf event to be fired on each\n> +iteration. Instead, a timer is registered with the telemetry layer and\n> +the data will be included in a \"timers\" sub-section in the `cmd_exit` event.\n> +\n> +For example, a timer was added to do_read_index() and do_write_index()\n> +to measure the time spent reading and writing the index.\n> +\n> +{\n> + \"event_name\": \"cmd_exit\",\n> + \"argv\": [\n> + \"C:\\\\work\\\\gfw\\\\git.exe\",\n> + \"status\"\n> + ],\n> + ...\n> + \"timers\": {\n> + \"do_read_index\": {\n> + \"count\": 1,\n> + \"total\": 0.000740,\n> + \"min\": 0.000740,\n> + \"max\": 0.000740,\n> + \"avg\": 0.000740\n> + },\n> + \"do_write_index\": {\n> + \"count\": 1,\n> + \"total\": 0.004724,\n> + \"min\": 0.004724,\n> + \"max\": 0.004724,\n> + \"avg\": 0.004724\n> + }\n> + },\n> + \"exit_code\": 0,\n> + \"elapsed_time_core\": 0.049865,\n> + \"elapsed_time_total\": 0.049867,\n> + \"builtin\": {\n> + \"name\": \"status\"\n> + }\n> +}\n> +\n> +The `timers` structure contains a named member for each defined timer.\n> +Within each individual timer, we have:\n> + * `count` is the number of times it was started/stopped.\n> + * `total` is the total time the timer was running.\n> + * `min` is the shortest interval.\n> + * `max` is the longest interval.\n> + * `avg` is the average interval.\n\nI wonder if we should add \"median\" here, although that would (unless\nI've missed some clever trick) require keeping all the times around,\nsorting them and then picking the 50th percentile, whereas we can\ncompute a running min/avg/max. So maybe it should be optional (this can\nalways be added later).\n\nOnce we have that we can easily add 10/25/50/75/90/95/99th percentiles\nas well.\n\n> +Field: aux\n> +---------------\n> +\n> +The `aux` structure within the `cmd_exit` event contains additional\n> +information about the process. This is intended as a generic container for\n> +various fields, such as important config settings or repo data shape that\n> +may affect performance or help identify the repository for aggregation\n> +purposes.\n> +\n> +{\n> + \"event_name\": \"cmd_exit\",\n> + ...\n> + \"aux\": {\n> + \"remote_origin_url\": \"git@github.com:git/git.git\",\n> + \"index_count\": 3311,\n> + \"sparse_checkout_count\": 3\n> + },\n> + ...\n> +}\n> +\n> +Other fields (and even sub-structures) can be added to this container\n> +as needed.\n> +\n> +\n> +Telemetry Destination\n> +=====================\n> +\n> +Telemetry events are sent to a \"destination\". This can be a file or a\n> +plugin. Telemetry is disabled if a destination is not set.\n> +\n> +telemetry.path\n\nThere is the telemetry.plugin escape hatch noted below I can change it,\nbut I don't like the design of this, because:\n\n1) On POSIX systems (I don't know about Windows) if you O_APPPEND to\nsuch a file you're guaranteed at most PIPE_MAX bytes written to the file\nbefore there's no guarantee that your write is atomic, i.e. you will get\ninterleaved JSON if we ever write something bigger than that.\n\nFrom the spec above it seems like this will be unlikely in practice for\nmost simple invocations, but with \"aux\" payloads and a sufficient amount\nof child processes & timers I can see us easily going over PIPE_MAX,\nwhich is commonly 4096 (e.g. the linux default).\n\n2) It makes it hard to process these events piecemeal and in parallel,\ni.e. imagine having all the dev machines NFS mount some logging FS where\nthey all write to this file, now I need to periodically move that file,\nor ensure the config is set so we write to a daily/hourly file etc.\n\nA tiny modification on this would address #1 and #2. We would make\ntelemetry.path the path to a directory where metrics are written, and\nthen write e.g. //, similar to how we write loose objects.\n\nThen a bunch of processes could write to those files in parallel without\nfear of clobbering anything, and anything consuming them could easily do\nso with a standard readdir() + stat() + rename(F -> F.doing) &&\nunlink(F.doing) pattern.\n\nBut reading between the lines you're not planning to use this yourself\nat all, and we can always add a telemetry.directory later with this\nbehavior, leaving telemetry.path for a \"simple\" backend (while\nprominently advertising that PIPE_MAX caveat).\n\n> +If the config setting \"telemetry.path\" contains a pathname, telemetry\n> +events will be appended to that file using the builtin destination\n> +handler. (File rotation is beyond the scope of this design.)\n> +\n> +Events are written as a series of JSON records. When \"telemetry.pretty\"\n> +is false, each event record will be written on one line.\n> +\n> +(All of the examples in this document were prepared with \"telemetry.pretty\"\n> +set to true.)\n> +\n> +telemetry.plugin\n> +----------------\n> +\n> +If the config setting \"telemetry.plugin\" contains the pathname to a shared\n> +library, the library will be dynamically loaded during start up and events\n> +will be sent to it using the plugin API.\n> +\n> +This plugin model allows an organization to define a custom or private\n> +telemetry solution while using a stock version of Git.\n> +\n> +For example, on Windows, it allows telemetry events to go directly to the\n> +kernel via the plugin using the high performance Event Tracing for Windows\n> +(ETW) facility.\n> +\n> +The contrib/telemetry-plugin-examples directory contains two example\n> +plugins:\n> + * A trivial log to stderr\n> + * A trivial ETW writer\n\nMaybe we should also have a telemetry.hook for flexibility on systems\nwhere invoking processes isn't expensive :)\n\nThis is a bit of scope creep on what you're trying to accomplish, but I\nwonder if we could keep the requirements for some sort of general *.so\nhook facility in mind with this.\n\nOur hooks now do relatively simple things like take a stream on STDIN,\nget ENV vars, and inspect ARGV. Could we generalize this to using some\ncallback mechanism that provides those, so this could also be used for\nother existing hooks?\n\n> +GDPR and Privacy\n> +================\n> +\n> +The telemetry feature can log possibly sensitive user information (such as\n> +command line arguments, which may contain URLs, user names, and file names).\n> +\n> +The base telemetry feature can write telemetry data to a file on the system.\n> +\n> +The plugin facility can be used to publish the telemetry data to more general\n> +destinations (such as ETW or the network).\n> +\n> +In both cases, it is up to the user or system administrator to decide what\n> +is appropriate and sanitize the data accordingly before broadcasting it.\n"},{"id":"349720","messageId":"CACsJy8DYFfLdXLzdZOz1V2XC9hpGT0igMZ-WJVpWp_UvMEnOgA@mail.gmail.com","threadId":"48670","inReplyTo":"871sdhzkd2.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-06-08T15:46:17Z","receivedAt":"2018-06-08T15:46:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jun 8, 2018 at 11:40 AM, Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Thu, Jun 07 2018, Johannes Sixt wrote:\n>\n>> Am 07.06.2018 um 16:53 schrieb git@jeffhostetler.com:\n>>> From: Jeff Hostetler \n>>>\n>>> I've been working to add code to Git to optionally collect telemetry data.\n>>> The goal is to be able to collect performance data from Git commands and\n>>> allow it to be aggregated over a user community to find \"slow commands\".\n>>\n>> Seriously? \"add code to collect telemetry data\" said by somebody whose\n>> email address ends with @microsoft.com is very irritating. I really\n>> don't want to have yet another switch that I must check after every\n>> update that it is still off.\n>\n> To elaborate a bit on Jeff's reply (since this was discussed in more\n> detail at Git Merge this year), the point of this feature is not to ship\n> git.git with some default telemetry, but so that in-house users of git\n> like Microsoft, Dropbox, Booking.com etc. can build & configure their\n> internal versions of git to turn on telemetry for their own users.\n>\n> There's numerous in-house monkeypatches to git on various sites (at\n> least Microsoft & Dropbox reported having internal patches already).\n> Something like this facility would allow us to agree on some\n> implementation that could be shipped by default (but would be off by\n> default), those of us who'd make use of this feature already have \"root\"\n> on those user's machines, and control what git binary they use etc,\n> their /etc/gitconfig and so on.\n\nThis elaboration should have been in the commit message of the first\npatch (and I hope it will be in v2). You guys knew because it was\ndiscussed at Git Merge but the rest of us may not.. It would be much\nmore convincing to have something like this spelled out.\n-- \nDuy\n"},{"id":"349722","messageId":"688240ef-34a1-ee9a-215a-b4f9628e7c72@virtuell-zuhause.de","threadId":"48670","inReplyTo":"20180608090758.GA15112@sigill.intra.peff.net","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2018-06-08T16:00:28Z","receivedAt":"2018-06-08T16:00:43Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"Am 08.06.2018 um 11:07 schrieb Jeff King:\n> On Thu, Jun 07, 2018 at 11:10:52PM +0200, Johannes Sixt wrote:\n> \n>> Am 07.06.2018 um 16:53 schrieb git@jeffhostetler.com:\n>>> From: Jeff Hostetler \n>>>\n>>> I've been working to add code to Git to optionally collect telemetry data.\n>>> The goal is to be able to collect performance data from Git commands and\n>>> allow it to be aggregated over a user community to find \"slow commands\".\n>>\n>> Seriously? \"add code to collect telemetry data\" said by somebody whose email\n>> address ends with @microsoft.com is very irritating. I really don't want to\n>> have yet another switch that I must check after every update that it is\n>> still off.\n> \n> If you look at the design document, it's off by default and would write\n> to a file on the filesystem. That doesn't seem all that different from\n> GIT_TRACE.\n\nThe patch also includes the following part\n\n+telemetry.plugin\n+----------------\n+\n+If the config setting \"telemetry.plugin\" contains the pathname to a shared\n+library, the library will be dynamically loaded during start up and events\n+will be sent to it using the plugin API.\n+\n+This plugin model allows an organization to define a custom or private\n+telemetry solution while using a stock version of Git.\n+\n+For example, on Windows, it allows telemetry events to go directly to the\n+kernel via the plugin using the high performance Event Tracing for Windows\n+(ETW) facility.\n+\n+The contrib/telemetry-plugin-examples directory contains two example\n+plugins:\n+ * A trivial log to stderr\n+ * A trivial ETW writer\n\nwhich is not a file but, if enabled, some windows internal thingie where the data is gone/duplicated/sent out/whatever.\n\nI for my part would much rather prefer that to be a compile time option so that I don't need to check on every git update on windows if this is now enabled or not.\n"},{"id":"349743","messageId":"9ab3eec1-40c1-8543-e122-ed4ccfd367b4@kdbg.org","threadId":"48670","inReplyTo":"688240ef-34a1-ee9a-215a-b4f9628e7c72@virtuell-zuhause.de","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-06-08T22:01:00Z","receivedAt":"2018-06-08T22:01:07Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 08.06.2018 um 18:00 schrieb Thomas Braun:\n> I for my part would much rather prefer that to be a compile time\n> option so that I don't need to check on every git update on windows\n> if this is now enabled or not.\n\nThis exactly my concern, too! A compile-time option may make it a good \ndeal less worrisome.\n\n-- Hannes\n"},{"id":"349745","messageId":"87fu1w53af.fsf@evledraar.gmail.com","threadId":"48670","inReplyTo":"9ab3eec1-40c1-8543-e122-ed4ccfd367b4@kdbg.org","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-08T22:20:08Z","receivedAt":"2018-06-08T22:20:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jun 08 2018, Johannes Sixt wrote:\n\n> Am 08.06.2018 um 18:00 schrieb Thomas Braun:\n>> I for my part would much rather prefer that to be a compile time\n>> option so that I don't need to check on every git update on windows\n>> if this is now enabled or not.\n>\n> This exactly my concern, too! A compile-time option may make it a good\n> deal less worrisome.\n\nCan you elaborate on how someone who can maintain inject malicious code\ninto your git package + config would be thwarted by this being some\ncompile-time option, wouldn't they just compile it in?\n"},{"id":"349771","messageId":"CACsJy8BPhzs5M4peHN2HczmDxGmAuKZ0corzT66i+rJ2UQRTHQ@mail.gmail.com","threadId":"48670","inReplyTo":"87fu1w53af.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-06-09T05:03:53Z","receivedAt":"2018-06-09T05:10:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jun 9, 2018 at 12:22 AM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Fri, Jun 08 2018, Johannes Sixt wrote:\n>\n> > Am 08.06.2018 um 18:00 schrieb Thomas Braun:\n> >> I for my part would much rather prefer that to be a compile time\n> >> option so that I don't need to check on every git update on windows\n> >> if this is now enabled or not.\n> >\n> > This exactly my concern, too! A compile-time option may make it a good\n> > deal less worrisome.\n>\n> Can you elaborate on how someone who can maintain inject malicious code\n> into your git package + config would be thwarted by this being some\n> compile-time option, wouldn't they just compile it in?\n\n\nLook at this from a different angle. This is driven by the needs to\ncollect telemetry in _controlled_ environment (mostly server side, I\nguess) and it should be no problem to make custom builds there for\nyou. Not making it a compile-time option could force [1] linux distro\nto carry this function to everybody even if they don't use it (and\nit's kinda dangerous to misuse if you don't anonymize the data\nproperly). I also prefer this a compile time option.\n\n[1] Of course many distros can choose to patch it out. But it's the\nsame argument as bringing this option in in the first place: you guys\nalready have that code in private and now want to put it in stock git\nto reduce maintenance cost, why add extra cost on linux distro\nmaintenance?\n-- \nDuy\n"},{"id":"349775","messageId":"87bmck4gip.fsf@evledraar.gmail.com","threadId":"48670","inReplyTo":"CACsJy8BPhzs5M4peHN2HczmDxGmAuKZ0corzT66i+rJ2UQRTHQ@mail.gmail.com","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-06-09T06:31:58Z","receivedAt":"2018-06-09T06:32:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jun 09 2018, Duy Nguyen wrote:\n\n> On Sat, Jun 9, 2018 at 12:22 AM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>>\n>> On Fri, Jun 08 2018, Johannes Sixt wrote:\n>>\n>> > Am 08.06.2018 um 18:00 schrieb Thomas Braun:\n>> >> I for my part would much rather prefer that to be a compile time\n>> >> option so that I don't need to check on every git update on windows\n>> >> if this is now enabled or not.\n>> >\n>> > This exactly my concern, too! A compile-time option may make it a good\n>> > deal less worrisome.\n>>\n>> Can you elaborate on how someone who can maintain inject malicious code\n>> into your git package + config would be thwarted by this being some\n>> compile-time option, wouldn't they just compile it in?\n>\n>\n> Look at this from a different angle. This is driven by the needs to\n> collect telemetry in _controlled_ environment (mostly server side, I\n> guess) and it should be no problem to make custom builds there for\n> you.\n\nLet's say you're in a corporate environment with Linux, OSX and Windows\nboxes, but all of whom have some shared mounts provisioned & ability to\nship an /etc/gitconfig (wherever that lives on Windows).\n\nIt's much easier to just do that than figure out how to build a custom\nGit on all three platforms.\n\nI guess you might make the argument that \"that's good\", because in\npractice that'll mean that it's such a hassle that fewer administrators\nwill turn this on.\n\nBut I think that would be a loss, because that's taking the default view\nthat people with the rights (i.e. managed config access) to turn on\nsomething like this by default have nefarious motives, and we should do\nwhat we can to stop them.\n\nI don't think that's true, e.g. what I intend to use this for is:\n\n a) Getting aggregate data on what commands/switches are used, for\n purposes of training and prioritizing my upstream contributions.\n\n b) Aggregate performance data to figure out what hotspots to tackle in\n the code.\n\nThat's things that'll both benefit the users I'm responsible for, and\nthe wider git community.\n\n> Not making it a compile-time option could force [1] linux distro\n> to carry this function to everybody even if they don't use it (and\n> it's kinda dangerous to misuse if you don't anonymize the data\n> properly). I also prefer this a compile time option.\n\nSetting GIT_TRACE to a filename that you published is also similarly\ndangerous, so would setting up a trivial 4-line shell alias to wrap\n\"git\" and log what it's doing.\n\n> [1] Of course many distros can choose to patch it out. But it's the\n> same argument as bringing this option in in the first place: you guys\n> already have that code in private and now want to put it in stock git\n> to reduce maintenance cost, why add extra cost on linux distro\n> maintenance?\n\nBecause:\n\n1) I really don't see the basis for this argument that they'd need to\n patch it out, they're not patching out e.g. GIT_TRACE now, which has\n all the same sort of concerns, it's just a format that's more of a\n hassle to parse than this proposed format.\n\n2) I think you and Johannes are just seeing the \"telemetry\" part of\n this, but if you look past that all this *really* is is \"GIT_TRACE\n facility that doesn't suck to parse\".\n\n There's a lot of use-cases for that which have nothing to do with\n what this facility is originally written for, for example, a novice\n git user could turn it on and have it log in ~ somewhere, and then\n run some contrib script which analyzes his git usage and spews out\n suggestions (\"you use this command/option, but not this related\n useful command/option\").\n\n Users asking for help on the #git IRC channel or on this mailing list\n could turn this on if they have a problem, and paste it into some\n tool they could show to others to see exactly what they're doing /\n where it went wrong.\n"},{"id":"349776","messageId":"20180609065132.GD30224@sigill.intra.peff.net","threadId":"48670","inReplyTo":"CACsJy8BPhzs5M4peHN2HczmDxGmAuKZ0corzT66i+rJ2UQRTHQ@mail.gmail.com","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-09T06:51:32Z","receivedAt":"2018-06-09T06:51:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 09, 2018 at 07:03:53AM +0200, Duy Nguyen wrote:\n\n> > > Am 08.06.2018 um 18:00 schrieb Thomas Braun:\n> > >> I for my part would much rather prefer that to be a compile time\n> > >> option so that I don't need to check on every git update on windows\n> > >> if this is now enabled or not.\n> > >\n> > > This exactly my concern, too! A compile-time option may make it a good\n> > > deal less worrisome.\n> >\n> > Can you elaborate on how someone who can maintain inject malicious code\n> > into your git package + config would be thwarted by this being some\n> > compile-time option, wouldn't they just compile it in?\n> \n> Look at this from a different angle. This is driven by the needs to\n> collect telemetry in _controlled_ environment (mostly server side, I\n> guess) and it should be no problem to make custom builds there for\n> you. Not making it a compile-time option could force [1] linux distro\n> to carry this function to everybody even if they don't use it (and\n> it's kinda dangerous to misuse if you don't anonymize the data\n> properly). I also prefer this a compile time option.\n\nI actually think this could be useful for normal users, too. We have\nGIT_TRACE for dumping debug output, and we sometimes ask users to turn\nit on to help diagnose a problem (not to mention that we use it\nourselves).\n\nAFAICT this telemetry data is the same thing, but for performance. When\nsomebody says \"why does this command take so long\", wouldn't it be nice\nfor us to be able to tell them to flip a switch that will collect data\non which operations are taking a long time?\n\n> [1] Of course many distros can choose to patch it out. But it's the\n> same argument as bringing this option in in the first place: you guys\n> already have that code in private and now want to put it in stock git\n> to reduce maintenance cost, why add extra cost on linux distro\n> maintenance?\n\nWe don't do performance telemetry like this at GitHub, but we do collect\na few variables that we dump to a custom over a Unix socket (e.g., our\nupload-pack records clone vs fetch and shallow vs non-shallow for each\nrequest).\n\nIn my experience the maintenance burden is not in the \"connect to a\nsocket\" part, but the fact that you have to sprinkle the entry points\nthroughout the code (e.g., \"set 'cloning' flag to 1\" or \"I'm entering\nthe pack generation phase of the fetch\"). So I'd love to see us do that\nsprinkling _once_ where everyone can benefit, whether they want better\ntracing for debugging, telemetry across their installed base, or\nwhatever.\n\nThe mechanism to handle those calls is much easier to plug in and out,\nthen. And I don't have a huge preference for compile-time versus\nrun-time, or whether bits are in-tree or out-of-tree. Obviously we'd\nhave some basic \"write to stderr or a file\" consumer in-tree.\n\nFor myself, I'm happy with compile-time (I'm instrumenting the server\nside, so I really only care about my specific build) and out-of-tree\n(the protocol to our custom daemon consumer is not something anybody\nelse would care about anyway).\n\nFor people collecting from clients, I imagine it's more convenient to be\nable to let them use official builds and just tweak a knob, even if they\n_could_ build a custom Git and push it out to everybody. I don't know\nanything about Windows event tracing, but if it's a standard facility\nthen I doubt we're talking about a huge maintenance burden to have it\nin-tree as a configurable option.\n\nSo IMHO that really leaves the \"is it too scary to have a config knob\nthat lets tracing go to this event facility versus to a file\"? I guess I\njust don't see it, as long as the user has to enable it explicitly. That\nseems like complaining that GIT_TRACE could go to syslog. If you don't\nwant to store it, then don't turn on the feature.\n\n-Peff\n"},{"id":"349777","messageId":"900a34bb-4d8b-4a22-a0f5-1d2f6df84945@kdbg.org","threadId":"48670","inReplyTo":"87fu1w53af.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-06-09T06:56:00Z","receivedAt":"2018-06-09T06:56:05Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 09.06.2018 um 00:20 schrieb Ævar Arnfjörð Bjarmason:\n> \n> On Fri, Jun 08 2018, Johannes Sixt wrote:\n> \n>> Am 08.06.2018 um 18:00 schrieb Thomas Braun:\n>>> I for my part would much rather prefer that to be a compile time\n>>> option so that I don't need to check on every git update on windows\n>>> if this is now enabled or not.\n>>\n>> This exactly my concern, too! A compile-time option may make it a good\n>> deal less worrisome.\n> \n> Can you elaborate on how someone who can maintain inject malicious code\n> into your git package + config would be thwarted by this being some\n> compile-time option, wouldn't they just compile it in?\n\nOf course they can. But would we, the Git community do that?\n\n From the design document:\n\n > The goal of the telemetry feature is to be able to gather usage data\n > across a group of production users to identify real-world performance\n > problems in production. Additionally, it might help identify common\n > user errors and guide future user training.\n\nThe goal to gather usage data may be valid for a small subset of Git \ninstallations. But it is wrong to put this into the software itself, in \nparticular when the implementations includes scary things like loading \nunspecified dynamic libraries:\n\n > If the config setting \"telemetry.plugin\" contains the pathname to a\n > shared library, the library will be dynamically loaded during start up\n > and events will be sent to it using the plugin API.\n\nWhen you want usage data, ask your users for feedback. Look over their \nshoulders. But do not ask the software itself to gather usage data. It \nwill be abused.\n\nDo not offer open source software that has a \"call-home\" method built-in.\n\nIf you want to peek into the workplaces of YOUR users, then monkey-patch \nsurvaillance into YOUR version of Git. But please do not burden the rest \nof us.\n\n-- Hannes\n"},{"id":"349778","messageId":"20180609065635.GE30224@sigill.intra.peff.net","threadId":"48670","inReplyTo":"87bmck4gip.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-09T06:56:35Z","receivedAt":"2018-06-09T06:56:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 09, 2018 at 08:31:58AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> 1) I really don't see the basis for this argument that they'd need to\n> patch it out, they're not patching out e.g. GIT_TRACE now, which has\n> all the same sort of concerns, it's just a format that's more of a\n> hassle to parse than this proposed format.\n> \n> 2) I think you and Johannes are just seeing the \"telemetry\" part of\n> this, but if you look past that all this *really* is is \"GIT_TRACE\n> facility that doesn't suck to parse\".\n\nSo that actually takes me to another question, which is: instead of\nintroducing a new \"telemetry\" feature, could we make GIT_TRACE suck less\nto parse?\n\nE.g., could we have a flag or environment variable to have the existing\ntraces output JSON? I guess right now they're inherently free-form via\ntrace_printf, so it would involve adding some structured interface\ncalls. Which is more or less what I guess JeffH's proposed feature to\nlook like.\n\n-Peff\n"},{"id":"349779","messageId":"208bcf92-cc23-d63d-3dfc-441e960e1e02@kdbg.org","threadId":"48670","inReplyTo":"20180609065132.GD30224@sigill.intra.peff.net","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-06-09T07:04:16Z","receivedAt":"2018-06-09T07:04:20Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 09.06.2018 um 08:51 schrieb Jeff King:\n> I actually think this could be useful for normal users, too. We have\n> GIT_TRACE for dumping debug output, and we sometimes ask users to turn\n> it on to help diagnose a problem (not to mention that we use it\n> ourselves).\n\nThe BIG difference is in \"we ask the users\". Asking for a trace is a \none-shot thing; this telemetry thing is obviously intended for long-term \nsurvaillance.\n\n> AFAICT this telemetry data is the same thing, but for performance. When\n> somebody says \"why does this command take so long\", wouldn't it be nice\n> for us to be able to tell them to flip a switch that will collect data\n> on which operations are taking a long time?\n\nWhy do we need long-term survaillance to answer this question and why \ncan it not be made a mode of GIT_TRACE?\n\n-- Hannes\n"},{"id":"349780","messageId":"20180609073132.GA1554@sigill.intra.peff.net","threadId":"48670","inReplyTo":"208bcf92-cc23-d63d-3dfc-441e960e1e02@kdbg.org","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-09T07:31:32Z","receivedAt":"2018-06-09T07:31:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 09, 2018 at 09:04:16AM +0200, Johannes Sixt wrote:\n\n> > AFAICT this telemetry data is the same thing, but for performance. When\n> > somebody says \"why does this command take so long\", wouldn't it be nice\n> > for us to be able to tell them to flip a switch that will collect data\n> > on which operations are taking a long time?\n> \n> Why do we need long-term survaillance to answer this question and why can it\n> not be made a mode of GIT_TRACE?\n\nI guess I don't see how this isn't simply a mode of GIT_TRACE.\n\n-Peff\n"},{"id":"349781","messageId":"CACsJy8DrtAdwH0dOjptJ0JDQ_gOeiTVbRknui4gM5P85Y5JZow@mail.gmail.com","threadId":"48670","inReplyTo":"87bmck4gip.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-06-09T07:31:57Z","receivedAt":"2018-06-09T07:32:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jun 9, 2018 at 8:32 AM Ævar Arnfjörð Bjarmason wrote:\n> Let's say you're in a corporate environment with Linux, OSX and Windows\n> boxes, but all of whom have some shared mounts provisioned & ability to\n> ship an /etc/gitconfig (wherever that lives on Windows).\n>\n> It's much easier to just do that than figure out how to build a custom\n> Git on all three platforms.\n\nLet's say I don't care about making it easier on corporate\nenvironment. You have resources to do that.\n\n> > Not making it a compile-time option could force [1] linux distro\n> > to carry this function to everybody even if they don't use it (and\n> > it's kinda dangerous to misuse if you don't anonymize the data\n> > properly). I also prefer this a compile time option.\n>\n> Setting GIT_TRACE to a filename that you published is also similarly\n> dangerous, so would setting up a trivial 4-line shell alias to wrap\n> \"git\" and log what it's doing.\n>\n> > [1] Of course many distros can choose to patch it out. But it's the\n> > same argument as bringing this option in in the first place: you guys\n> > already have that code in private and now want to put it in stock git\n> > to reduce maintenance cost, why add extra cost on linux distro\n> > maintenance?\n>\n> Because:\n>\n> 1) I really don't see the basis for this argument that they'd need to\n> patch it out, they're not patching out e.g. GIT_TRACE now, which has\n> all the same sort of concerns, it's just a format that's more of a\n> hassle to parse than this proposed format.\n>\n> 2) I think you and Johannes are just seeing the \"telemetry\" part of\n> this, but if you look past that all this *really* is is \"GIT_TRACE\n> facility that doesn't suck to parse\".\n\nIf it's simply an improvement over GIT_TRACE, then I have no problem\nwith that. That is:\n\n- there is no new, permanent way to turn this one like config keys\n\n- there's no plugin support or whatever for keeping output. Keep it to\neither a file or a file descriptor like we do with GIT_TRACE.\n\n> There's a lot of use-cases for that which have nothing to do with\n> what this facility is originally written for, for example, a novice\n> git user could turn it on and have it log in ~ somewhere, and then\n> run some contrib script which analyzes his git usage and spews out\n> suggestions (\"you use this command/option, but not this related\n> useful command/option\").\n>\n> Users asking for help on the #git IRC channel or on this mailing list\n> could turn this on if they have a problem, and paste it into some\n> tool they could show to others to see exactly what they're doing /\n> where it went wrong.\n\nAnd they can use GIT_TRACE now. If GIT_TRACE does not give enough info\n(e.g. too few trace points), add them. This new proposal is more about\nautomation. With GIT_TRACE, which is more human friendly, I could skim\nover the output and remove sensitive info before I send it out for\nhelp. Machine-friendly format (even json) tends to be a lot more\ncomplex and harder to read/filter this way.\n-- \nDuy\n"},{"id":"349824","messageId":"nycvar.QRO.7.76.6.1806092200490.77@tvgsbejvaqbjf.bet","threadId":"48670","inReplyTo":"20180609065635.GE30224@sigill.intra.peff.net","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-06-09T20:05:49Z","receivedAt":"2018-06-09T20:06:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\n\nOn Sat, 9 Jun 2018, Jeff King wrote:\n\n> On Sat, Jun 09, 2018 at 08:31:58AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> > 1) I really don't see the basis for this argument that they'd need to\n> > patch it out, they're not patching out e.g. GIT_TRACE now, which has\n> > all the same sort of concerns, it's just a format that's more of a\n> > hassle to parse than this proposed format.\n> >\n> > 2) I think you and Johannes are just seeing the \"telemetry\" part of\n> > this, but if you look past that all this *really* is is \"GIT_TRACE\n> > facility that doesn't suck to parse\".\n> \n> So that actually takes me to another question, which is: instead of\n> introducing a new \"telemetry\" feature, could we make GIT_TRACE suck less\n> to parse?\n\nThat's a different question, and even a different concern.\n\nPlease understand that this feature is needed for our own in-house use, to\nbe able to help our users pro-actively. We really want to be able to\ncontact a user when they struggle with the performance of any given Git\ncommand, before they have to think of reaching out for assistance.\n\nWe discussed this plan at the contributor summit at GitMerge, and IIRC at\nleast Autodesk and DropBox were interested (but of course, the changes of\nboth Lars' and Alex' roles might make them less interested now), hence\nJeffHost wanted to contribute this, for the benefit of the larger Git\ncommunity.\n\n> E.g., could we have a flag or environment variable to have the existing\n> traces output JSON? I guess right now they're inherently free-form via\n> trace_printf, so it would involve adding some structured interface\n> calls. Which is more or less what I guess JeffH's proposed feature to\n> look like.\n\nI think that is a much larger project than what JeffHost proposed, and\nwould unfortunately put too much of a brake on his project.\n\nCiao,\nDscho"},{"id":"349828","messageId":"nycvar.QRO.7.76.6.1806092241180.77@tvgsbejvaqbjf.bet","threadId":"48670","inReplyTo":"900a34bb-4d8b-4a22-a0f5-1d2f6df84945@kdbg.org","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-06-09T20:43:37Z","receivedAt":"2018-06-09T20:44:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Sat, 9 Jun 2018, Johannes Sixt wrote:\n\n> Am 09.06.2018 um 00:20 schrieb Ævar Arnfjörð Bjarmason:\n> > \n> > On Fri, Jun 08 2018, Johannes Sixt wrote:\n> > \n> > > Am 08.06.2018 um 18:00 schrieb Thomas Braun:\n> > > > I for my part would much rather prefer that to be a compile time\n> > > > option so that I don't need to check on every git update on windows\n> > > > if this is now enabled or not.\n> > >\n> > > This exactly my concern, too! A compile-time option may make it a good\n> > > deal less worrisome.\n> > \n> > Can you elaborate on how someone who can maintain inject malicious code\n> > into your git package + config would be thwarted by this being some\n> > compile-time option, wouldn't they just compile it in?\n> \n> Of course they can. But would we, the Git community do that?\n> \n> From the design document:\n> \n> > The goal of the telemetry feature is to be able to gather usage data\n> > across a group of production users to identify real-world performance\n> > problems in production. Additionally, it might help identify common\n> > user errors and guide future user training.\n> \n> The goal to gather usage data may be valid for a small subset of Git\n> installations. But it is wrong to put this into the software itself, in\n> particular when the implementations includes scary things like loading\n> unspecified dynamic libraries:\n> \n> > If the config setting \"telemetry.plugin\" contains the pathname to a\n> > shared library, the library will be dynamically loaded during start up\n> > and events will be sent to it using the plugin API.\n> \n> When you want usage data, ask your users for feedback. Look over their\n> shoulders. But do not ask the software itself to gather usage data. It will be\n> abused.\n> \n> Do not offer open source software that has a \"call-home\" method built-in.\n> \n> If you want to peek into the workplaces of YOUR users, then monkey-patch\n> survaillance into YOUR version of Git. But please do not burden the rest of\n> us.\n\nWe already offer hooks. You can do anything with those hooks. Even, if you\ndo not pay close attention, to transfer all your bitcoin to a specific\naccount.\n\nI agree with Peff: this is something you as a user need to be aware of,\nand need to make sure you configure your Git just like you want. As long\nas this is a purely opt-in feature, it is useful and helpful.\n\nWe do need it in-house, for the many thousands of Git users we try to\nsupport with a relatively small team of Git developers.\n\nCiao,\nDscho"},{"id":"349835","messageId":"410f0fee-010c-c178-224c-e47ae0b0dda6@kdbg.org","threadId":"48670","inReplyTo":"nycvar.QRO.7.76.6.1806092241180.77@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-06-09T22:44:25Z","receivedAt":"2018-06-09T22:44:32Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 09.06.2018 um 22:43 schrieb Johannes Schindelin:\n> On Sat, 9 Jun 2018, Johannes Sixt wrote:\n>> When you want usage data, ask your users for feedback. Look over their\n>> shoulders. But do not ask the software itself to gather usage data. It will be\n>> abused.\n>>\n>> Do not offer open source software that has a \"call-home\" method built-in.\n>>\n>> If you want to peek into the workplaces of YOUR users, then monkey-patch\n>> survaillance into YOUR version of Git. But please do not burden the rest of\n>> us.\n> \n> We already offer hooks. You can do anything with those hooks. Even, if you\n> do not pay close attention, to transfer all your bitcoin to a specific\n> account.\n> \n> I agree with Peff: this is something you as a user need to be aware of,\n> and need to make sure you configure your Git just like you want. As long\n> as this is a purely opt-in feature, it is useful and helpful.\n\nThe problem with this feature is not so much that it enables someone to \ndo bad things, but that it is specifically targeted at recording *how \nusers use Git*.\n\n> We do need it in-house, for the many thousands of Git users we try to\n> support with a relatively small team of Git developers.\n\nThen please build your private version and sell it to your thousands of \nGit users. \"in-house\" == Microsoft? Small team of Git developers? So, \ninstead of shelling out the bucks for this extra burden, they put the \nburden on the Community?\n\n-- Hannes\n"},{"id":"349839","messageId":"20180610000056.GD38834@genre.crustytoothpaste.net","threadId":"48670","inReplyTo":"900a34bb-4d8b-4a22-a0f5-1d2f6df84945@kdbg.org","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-06-10T00:00:57Z","receivedAt":"2018-06-10T00:01:08Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sat, Jun 09, 2018 at 08:56:00AM +0200, Johannes Sixt wrote:\n> Am 09.06.2018 um 00:20 schrieb Ævar Arnfjörð Bjarmason:\n> > \n> > On Fri, Jun 08 2018, Johannes Sixt wrote:\n> > Can you elaborate on how someone who can maintain inject malicious code\n> > into your git package + config would be thwarted by this being some\n> > compile-time option, wouldn't they just compile it in?\n> \n> Of course they can. But would we, the Git community do that?\n> \n> From the design document:\n> \n> > The goal of the telemetry feature is to be able to gather usage data\n> > across a group of production users to identify real-world performance\n> > problems in production. Additionally, it might help identify common\n> > user errors and guide future user training.\n> \n> The goal to gather usage data may be valid for a small subset of Git\n> installations. But it is wrong to put this into the software itself, in\n> particular when the implementations includes scary things like loading\n> unspecified dynamic libraries:\n> \n> > If the config setting \"telemetry.plugin\" contains the pathname to a\n> > shared library, the library will be dynamically loaded during start up\n> > and events will be sent to it using the plugin API.\n> \n> When you want usage data, ask your users for feedback. Look over their\n> shoulders. But do not ask the software itself to gather usage data. It will\n> be abused.\n> \n> Do not offer open source software that has a \"call-home\" method built-in.\n> \n> If you want to peek into the workplaces of YOUR users, then monkey-patch\n> survaillance into YOUR version of Git. But please do not burden the rest of\n> us.\n\nI understand there's an interest in supporting the most people with the\nfewest amount of staff. I'm certainly in the situation where I, with\nonly minimal assistance, support every Git user in my division of the\ncompany, regardless of technical ability, and I know how overwhelming\nthat can be. (Burnout, I can tell you, is a thing.)\n\nI also have to look at this issue from the interests of what is best for\nthe FLOSS community and for users as a whole. Adding in functionality\nthat sends off usage data from a command-line tool, especially one that\nis as widely used as Git is, is not in the interests of users as a\nwhole, nor is it common practice in FLOSS tools.\n\nAs a highly capable and technical user, I would find it very undesirable\nto have my development tools reporting data like this, even if it is to\nmake my experience better.\n\nThe ability to load arbitrary libraries makes me concerned about people\nusing this to spirit away personal or company data or to subtly steal\ndata in a rootkit-like situation. These are real threats in the kinds\nof environments I distribute to in my work role.\n\nI agree with Duy's point of view that GIT_TRACE-level output to a file\ndescriptor or file is fine, but a persistently enabled feature is not.\n\nI expect this feature, if implemented, would be patched out of Debian's\nGit, and it would be patched out of any Git I would distribute in my\nwork role for legal and ethical reasons.\n\nAs developers, we have a duty to be mindful of how our software can be\nmisused and abused and try to avoid that when possible. I don't think\nthis feature is on the right side of that balance.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"349874","messageId":"20180611055639.GA28598@sigill.intra.peff.net","threadId":"48670","inReplyTo":"nycvar.QRO.7.76.6.1806092200490.77@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-11T05:56:39Z","receivedAt":"2018-06-11T05:56:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 09, 2018 at 10:05:49PM +0200, Johannes Schindelin wrote:\n\n> > E.g., could we have a flag or environment variable to have the existing\n> > traces output JSON? I guess right now they're inherently free-form via\n> > trace_printf, so it would involve adding some structured interface\n> > calls. Which is more or less what I guess JeffH's proposed feature to\n> > look like.\n> \n> I think that is a much larger project than what JeffHost proposed, and\n> would unfortunately put too much of a brake on his project.\n\nI definitely don't want to stall somebody else's momentum with a bunch\nof what-if's. But I also don't want to end up down the road with two\nnearly-identical systems for tracing information. That's confusing to\nusers, and to developers who must choose which system to use for any new\ntracing information they add.\n\nSo I think it's worth at least giving a little thought to how we might\nleverage similarities between the trace system and this. Even if we\ndon't implement it now, it would be nice to have a vague sense of how\nthey could grow together in the long run.\n\n-Peff\n"},{"id":"349875","messageId":"20180611060832.GB28598@sigill.intra.peff.net","threadId":"48670","inReplyTo":"410f0fee-010c-c178-224c-e47ae0b0dda6@kdbg.org","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-11T06:08:33Z","receivedAt":"2018-06-11T06:08:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 10, 2018 at 12:44:25AM +0200, Johannes Sixt wrote:\n\n> > I agree with Peff: this is something you as a user need to be aware of,\n> > and need to make sure you configure your Git just like you want. As long\n> > as this is a purely opt-in feature, it is useful and helpful.\n> \n> The problem with this feature is not so much that it enables someone to do\n> bad things, but that it is specifically targeted at recording *how users use\n> Git*.\n\nI think one issue here is that we are not looking at concrete patches.\nSo for instance, I've seen a claim that Git should never have a way to\nturn on tracing all the time. But at GitHub we have found it useful to\nhave a config option to log the sha1 of every object that is dropped by\ngit-prune or by \"repack -ad\". It's helped both as a developer (tracking\ndown races or bugs in our code) and as an administrator (figuring out\nwhere a corruption was introduced). It needs to be on all the time to be\nuseful, since the point is to have an audit trail to look at _after_ a\nbad thing happens.\n\nThat's something we do completely on the server side; I don't think\nthere are any privacy or \"spying\" issues there. And I don't think it's a\nhuge maintenance burden. Inside the existing code, it's literally a\none-line \"log this\" (the log code itself is a hundred or so lines in its\nown file).\n\nNow most users probably don't care that much about this use case. And\nI'm OK to apply it as a custom patch. But doesn't it seem like that's\nsomething other people hosting Git repos might want? Or that the concept\nmight extend to other loggable items that _are_ interesting on the\nclient side?\n\nThat's why I think it is worth taking this step-by-step. Let's log more\nthings. Let's make enabling tracing more flexible. Those are hopefully\nuncontentious and universally useful. If you want to draw the line on\n\"spying\", then I think the right place to draw it is when somebody wants\nto ship code to actually move those logs out of the user's control.\n\n-Peff\n"},{"id":"349876","messageId":"20180611061441.GC28598@sigill.intra.peff.net","threadId":"48670","inReplyTo":"20180610000056.GD38834@genre.crustytoothpaste.net","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-11T06:14:41Z","receivedAt":"2018-06-11T06:14:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 10, 2018 at 12:00:57AM +0000, brian m. carlson wrote:\n\n> I also have to look at this issue from the interests of what is best for\n> the FLOSS community and for users as a whole. Adding in functionality\n> that sends off usage data from a command-line tool, especially one that\n> is as widely used as Git is, is not in the interests of users as a\n> whole, nor is it common practice in FLOSS tools.\n\nI'm not sure if this last statement is true. For instance, many tools\nhave crash-reporting functionality, and some contain more advanced\ntelemetry. E.g.:\n\n https://wiki.mozilla.org/Telemetry\n\nPersonally I do not like this sort of data collection at all, and never\nenable it. And I would be highly suspicious of any FLOSS tool that\nshipped with it enabled by default. But it seems like at least some\nprojects _do_ find it useful, and enough people enable it to make it\nworth carrying as a feature.\n\n-Peff\n"},{"id":"349880","messageId":"20180611083058.GA1592@sigill.intra.peff.net","threadId":"48670","inReplyTo":"20180611061441.GC28598@sigill.intra.peff.net","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-11T08:30:58Z","receivedAt":"2018-06-11T08:31:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 11, 2018 at 02:14:41AM -0400, Jeff King wrote:\n\n> On Sun, Jun 10, 2018 at 12:00:57AM +0000, brian m. carlson wrote:\n> \n> > I also have to look at this issue from the interests of what is best for\n> > the FLOSS community and for users as a whole. Adding in functionality\n> > that sends off usage data from a command-line tool, especially one that\n> > is as widely used as Git is, is not in the interests of users as a\n> > whole, nor is it common practice in FLOSS tools.\n> \n> I'm not sure if this last statement is true. For instance, many tools\n> have crash-reporting functionality, and some contain more advanced\n> telemetry. E.g.:\n> \n> https://wiki.mozilla.org/Telemetry\n> \n> Personally I do not like this sort of data collection at all, and never\n> enable it. And I would be highly suspicious of any FLOSS tool that\n> shipped with it enabled by default. But it seems like at least some\n> projects _do_ find it useful, and enough people enable it to make it\n> worth carrying as a feature.\n\nRe-reading what I wrote, I really want to emphasize something so that it\nisn't missed. I think the important feature here is user consent, and\nthat seems to be the line that projects like Mozilla use (default to\noff, and require explicit consent before sending anything off-host).\n\nWe don't have to use that line. I think \"do not collect even locally\" is\na reasonable line, because even collecting has performance and storage\nmanagement implications. And I don't think anybody is proposing to even\ncollect local telemetry without the user turning it on.\n\nThe line under discussion seems to be \"implement code to collect\nanything at all\". That's a valid line, too, but IMHO it ends up blocking\nuseful features (and I think we already violate it with things like\nGIT_TRACE!).\n\n-Peff\n"},{"id":"349985","messageId":"xmqq602oat49.fsf@gitster-ct.c.googlers.com","threadId":"48670","inReplyTo":"20180609065132.GD30224@sigill.intra.peff.net","subject":"Re: [RFC PATCH v1] telemetry design overview (part 1)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-12T16:04:38Z","receivedAt":"2018-06-12T16:04:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> In my experience the maintenance burden is not in the \"connect to a\n> socket\" part, but the fact that you have to sprinkle the entry points\n> throughout the code (e.g., \"set 'cloning' flag to 1\" or \"I'm entering\n> the pack generation phase of the fetch\"). So I'd love to see us do that\n> sprinkling _once_ where everyone can benefit, whether they want better\n> tracing for debugging, telemetry across their installed base, or\n> whatever.\n\nI tend to agree. Transport (or file) can stay outside the core of\nthis \"telemetry\" thing---agreeing on what and when to trace, and how\nthe trace is represented, and having an API and solid guideline,\nwould allow us to annotate the code just once and get useful data in\na consistent way.\n\n> The mechanism to handle those calls is much easier to plug in and out,\n> then. And I don't have a huge preference for compile-time versus\n> run-time, or whether bits are in-tree or out-of-tree. Obviously we'd\n> have some basic \"write to stderr or a file\" consumer in-tree.\n\nIf it makes readers fearful of big brother feel safer, I think we\nprobably can add code that is runtime controllable that is compiled\nin conditionally ;-)\n\nI do not quite buy \"Big brothers who want this for their own in\nhouse use case can afford to patch\" argument, by the way. If\nanything, having a solid tracing mechanism (to which existing\nGIT_TRACE support _should_ be able to serve as a starting point)\nwould mean small budget guys do not have to do their own investing\nto collect useful data over their customer base (I am seeing an\nanalog in \"popcon\", opt-in stats of installed package in a distro,\nwhich we can view as \"telemetry data for distro installer program\").\n\n"}]}