[PATCH] trace2: increment event format version

[PATCH] trace2: increment event format version

[Josh Steadmon]

In 64bc752 (trace2: add trace2_child_ready() to report on background
children, 2021-09-20), we added a new "child_ready" event. In
Documentation/technical/api-trace2.txt, we promise that adding a new
event type will result in incrementing the trace2 event format version
number, but this was not done. Correct this in code & docs.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 Documentation/technical/api-trace2.txt | 4 ++--
 trace2/tr2_tgt_event.c                 | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt
index ef7fe02a8f..bb13ca3db8 100644
--- a/Documentation/technical/api-trace2.txt
+++ b/Documentation/technical/api-trace2.txt
@@ -128,7 +128,7 @@ yields
 
 ------------
 $ cat ~/log.event
-{"event":"version","sid":"sid":"20190408T191610.507018Z-H9b68c35f-P000059a8","thread":"main","time":"2019-01-16T17:28:42.620713Z","file":"common-main.c","line":38,"evt":"2","exe":"2.20.1.155.g426c96fcdb"}
+{"event":"version","sid":"sid":"20190408T191610.507018Z-H9b68c35f-P000059a8","thread":"main","time":"2019-01-16T17:28:42.620713Z","file":"common-main.c","line":38,"evt":"3","exe":"2.20.1.155.g426c96fcdb"}
 {"event":"start","sid":"20190408T191610.507018Z-H9b68c35f-P000059a8","thread":"main","time":"2019-01-16T17:28:42.621027Z","file":"common-main.c","line":39,"t_abs":0.001173,"argv":["git","version"]}
 {"event":"cmd_name","sid":"20190408T191610.507018Z-H9b68c35f-P000059a8","thread":"main","time":"2019-01-16T17:28:42.621122Z","file":"git.c","line":432,"name":"version","hierarchy":"version"}
 {"event":"exit","sid":"20190408T191610.507018Z-H9b68c35f-P000059a8","thread":"main","time":"2019-01-16T17:28:42.621236Z","file":"git.c","line":662,"t_abs":0.001227,"code":0}
@@ -391,7 +391,7 @@ only present on the "start" and "atexit" events.
 {
 	"event":"version",
 	...
-	"evt":"2",		       # EVENT format version
+	"evt":"3",		       # EVENT format version
 	"exe":"2.20.1.155.g426c96fcdb" # git version
 }
 ------------
diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
index 70cfc2f77c..3a0014417c 100644
--- a/trace2/tr2_tgt_event.c
+++ b/trace2/tr2_tgt_event.c
@@ -20,7 +20,7 @@ static struct tr2_dst tr2dst_event = { TR2_SYSENV_EVENT, 0, 0, 0, 0 };
  * a new field to an existing event, do not require an increment to the EVENT
  * format version.
  */
-#define TR2_EVENT_VERSION "2"
+#define TR2_EVENT_VERSION "3"
 
 /*
  * Region nesting limit for messages written to the event target.

base-commit: e9e5ba39a78c8f5057262d49e261b42a8660d5b9
-- 
2.34.0.rc1.387.gb447b232ab-goog

Re: [PATCH] trace2: increment event format version

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> In 64bc752 (trace2: add trace2_child_ready() to report on background
> children, 2021-09-20), we added a new "child_ready" event. In
> Documentation/technical/api-trace2.txt, we promise that adding a new
> event type will result in incrementing the trace2 event format version
> number, but this was not done. Correct this in code & docs.
>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  Documentation/technical/api-trace2.txt | 4 ++--
>  trace2/tr2_tgt_event.c                 | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)

Hmph, it seems to me that this is better done before the release,
or am I mistaken?


>
> diff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt
> index ef7fe02a8f..bb13ca3db8 100644
> --- a/Documentation/technical/api-trace2.txt
> +++ b/Documentation/technical/api-trace2.txt
> @@ -128,7 +128,7 @@ yields
>  
>  ------------
>  $ cat ~/log.event
> -{"event":"version","sid":"sid":"20190408T191610.507018Z-H9b68c35f-P000059a8","thread":"main","time":"2019-01-16T17:28:42.620713Z","file":"common-main.c","line":38,"evt":"2","exe":"2.20.1.155.g426c96fcdb"}
> +{"event":"version","sid":"sid":"20190408T191610.507018Z-H9b68c35f-P000059a8","thread":"main","time":"2019-01-16T17:28:42.620713Z","file":"common-main.c","line":38,"evt":"3","exe":"2.20.1.155.g426c96fcdb"}
>  {"event":"start","sid":"20190408T191610.507018Z-H9b68c35f-P000059a8","thread":"main","time":"2019-01-16T17:28:42.621027Z","file":"common-main.c","line":39,"t_abs":0.001173,"argv":["git","version"]}
>  {"event":"cmd_name","sid":"20190408T191610.507018Z-H9b68c35f-P000059a8","thread":"main","time":"2019-01-16T17:28:42.621122Z","file":"git.c","line":432,"name":"version","hierarchy":"version"}
>  {"event":"exit","sid":"20190408T191610.507018Z-H9b68c35f-P000059a8","thread":"main","time":"2019-01-16T17:28:42.621236Z","file":"git.c","line":662,"t_abs":0.001227,"code":0}
> @@ -391,7 +391,7 @@ only present on the "start" and "atexit" events.
>  {
>  	"event":"version",
>  	...
> -	"evt":"2",		       # EVENT format version
> +	"evt":"3",		       # EVENT format version
>  	"exe":"2.20.1.155.g426c96fcdb" # git version
>  }
>  ------------
> diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
> index 70cfc2f77c..3a0014417c 100644
> --- a/trace2/tr2_tgt_event.c
> +++ b/trace2/tr2_tgt_event.c
> @@ -20,7 +20,7 @@ static struct tr2_dst tr2dst_event = { TR2_SYSENV_EVENT, 0, 0, 0, 0 };
>   * a new field to an existing event, do not require an increment to the EVENT
>   * format version.
>   */
> -#define TR2_EVENT_VERSION "2"
> +#define TR2_EVENT_VERSION "3"
>  
>  /*
>   * Region nesting limit for messages written to the event target.
>
> base-commit: e9e5ba39a78c8f5057262d49e261b42a8660d5b9

Re: [PATCH] trace2: increment event format version

[Josh Steadmon]

On 2021.11.11 15:03, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
> 
> > In 64bc752 (trace2: add trace2_child_ready() to report on background
> > children, 2021-09-20), we added a new "child_ready" event. In
> > Documentation/technical/api-trace2.txt, we promise that adding a new
> > event type will result in incrementing the trace2 event format version
> > number, but this was not done. Correct this in code & docs.
> >
> > Signed-off-by: Josh Steadmon <steadmon@google.com>
> > ---
> >  Documentation/technical/api-trace2.txt | 4 ++--
> >  trace2/tr2_tgt_event.c                 | 2 +-
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> Hmph, it seems to me that this is better done before the release,
> or am I mistaken?

Ideally yes, although I am not sure if there is anyone using traces who
strongly depends on the accuracy of the evt field. For release-blocking
fixes (for lack of a better term), should I have sent this patch
differently?

Re: [PATCH] trace2: increment event format version

[Junio C Hamano]

Josh Steadmon <steadmon@google.com> writes:

> On 2021.11.11 15:03, Junio C Hamano wrote:
>> Josh Steadmon <steadmon@google.com> writes:
>> 
>> > In 64bc752 (trace2: add trace2_child_ready() to report on background
>> > children, 2021-09-20), we added a new "child_ready" event. In
>> > Documentation/technical/api-trace2.txt, we promise that adding a new
>> > event type will result in incrementing the trace2 event format version
>> > number, but this was not done. Correct this in code & docs.
>> >
>> > Signed-off-by: Josh Steadmon <steadmon@google.com>
>> > ---
>> >  Documentation/technical/api-trace2.txt | 4 ++--
>> >  trace2/tr2_tgt_event.c                 | 2 +-
>> >  2 files changed, 3 insertions(+), 3 deletions(-)
>> 
>> Hmph, it seems to me that this is better done before the release,
>> or am I mistaken?
>
> Ideally yes, although I am not sure if there is anyone using traces who
> strongly depends on the accuracy of the evt field.

Relieving us from having to keep track of the actual users is the
point of documenting to making promises ;-)

> For release-blocking
> fixes (for lack of a better term), should I have sent this patch
> differently?

I do not think so.

Thanks.

Re: [PATCH] trace2: increment event format version

[Ævar Arnfjörð Bjarmason]

On Thu, Nov 11 2021, Junio C Hamano wrote:

> Josh Steadmon <steadmon@google.com> writes:
>
>> On 2021.11.11 15:03, Junio C Hamano wrote:
>>> Josh Steadmon <steadmon@google.com> writes:
>>> 
>>> > In 64bc752 (trace2: add trace2_child_ready() to report on background
>>> > children, 2021-09-20), we added a new "child_ready" event. In
>>> > Documentation/technical/api-trace2.txt, we promise that adding a new
>>> > event type will result in incrementing the trace2 event format version
>>> > number, but this was not done. Correct this in code & docs.
>>> >
>>> > Signed-off-by: Josh Steadmon <steadmon@google.com>
>>> > ---
>>> >  Documentation/technical/api-trace2.txt | 4 ++--
>>> >  trace2/tr2_tgt_event.c                 | 2 +-
>>> >  2 files changed, 3 insertions(+), 3 deletions(-)
>>> 
>>> Hmph, it seems to me that this is better done before the release,
>>> or am I mistaken?
>>
>> Ideally yes, although I am not sure if there is anyone using traces who
>> strongly depends on the accuracy of the evt field.
>
> Relieving us from having to keep track of the actual users is the
> point of documenting to making promises ;-)
>
>> For release-blocking
>> fixes (for lack of a better term), should I have sent this patch
>> differently?
>
> I do not think so.

Josh notes the "child_ready" event being new in this release, but the
same is true of the cmd_ancestry event added in 2f732bf15e6 (tr2: log
parent process name, 2021-07-21)

So yeah, with both of those it makes sense to have this for v2.34.0.

On the field itself I also wonder if it's useful at all. I'd think
anyone implementing a parser for the format would dispatch to a lookup
handling known keys, so having a version indicating "new keys here"
seems rather useless.

Re: [PATCH] trace2: increment event format version

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Josh notes the "child_ready" event being new in this release, but the
> same is true of the cmd_ancestry event added in 2f732bf15e6 (tr2: log
> parent process name, 2021-07-21)
>
> So yeah, with both of those it makes sense to have this for v2.34.0.

Yeah, thanks for an extra pair of eyes.

Re: [PATCH] trace2: increment event format version

[Jeff Hostetler]

On 11/12/21 5:33 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Nov 11 2021, Junio C Hamano wrote:
> 
>> Josh Steadmon <steadmon@google.com> writes:
>>
>>> On 2021.11.11 15:03, Junio C Hamano wrote:
>>>> Josh Steadmon <steadmon@google.com> writes:
>>>>
>>>>> In 64bc752 (trace2: add trace2_child_ready() to report on background
>>>>> children, 2021-09-20), we added a new "child_ready" event. In
>>>>> Documentation/technical/api-trace2.txt, we promise that adding a new
>>>>> event type will result in incrementing the trace2 event format version
>>>>> number, but this was not done. Correct this in code & docs.
>>>>>
...
> 
> On the field itself I also wonder if it's useful at all. I'd think
> anyone implementing a parser for the format would dispatch to a lookup
> handling known keys, so having a version indicating "new keys here"
> seems rather useless.
> 

That may be true, but it is easier to have a version number and
allow parsers to ignore it, than it is to not have a version number
and at some point in the future require parsers try to figure it
out. IMHO.

So far we've only added new event types (cmd_ancestry and child_ready)
and everything is in regular JSON forms, so parsing and dispatching
is relatively easy and the version number is not really needed.
But, if in the future we decide to change the contents within one of
those events, then the version number may be significant.

Thanks,
Jeff

Re: [PATCH] trace2: increment event format version

[Ævar Arnfjörð Bjarmason]

On Wed, Dec 01 2021, Jeff Hostetler wrote:

> On 11/12/21 5:33 PM, Ævar Arnfjörð Bjarmason wrote:
>> On Thu, Nov 11 2021, Junio C Hamano wrote:
>> 
>>> Josh Steadmon <steadmon@google.com> writes:
>>>
>>>> On 2021.11.11 15:03, Junio C Hamano wrote:
>>>>> Josh Steadmon <steadmon@google.com> writes:
>>>>>
>>>>>> In 64bc752 (trace2: add trace2_child_ready() to report on background
>>>>>> children, 2021-09-20), we added a new "child_ready" event. In
>>>>>> Documentation/technical/api-trace2.txt, we promise that adding a new
>>>>>> event type will result in incrementing the trace2 event format version
>>>>>> number, but this was not done. Correct this in code & docs.
>>>>>>
> ...
>> On the field itself I also wonder if it's useful at all. I'd think
>> anyone implementing a parser for the format would dispatch to a lookup
>> handling known keys, so having a version indicating "new keys here"
>> seems rather useless.
>> 
>
> That may be true, but it is easier to have a version number and
> allow parsers to ignore it, than it is to not have a version number
> and at some point in the future require parsers try to figure it
> out. IMHO.
>
> So far we've only added new event types (cmd_ancestry and child_ready)
> and everything is in regular JSON forms, so parsing and dispatching
> is relatively easy and the version number is not really needed.
> But, if in the future we decide to change the contents within one of
> those events, then the version number may be significant.

Yes, exactly. Versioning it makes sense, I'm commenting specifically on
the criteria for bumping the version. I.e. this bit in
trace2/tr2_tgt_event.c (with my comments)::
    
     * The version should be incremented if new event types are added

That seems entirely useless, i.e. just ignore unknown types. If you get
an unknown type you know something is new, no need for a version number
bump for this case.

It adds no new information you're not getting with "oh? a new field?".

     * if existing fields are removed, [...]

Useful, you don't always know if the absence of a field means it won't
be there at all, or if something else is up, particularly if the field
is optional. E.g. if we rename "error" to "exception" (or whatever) we
don't want an older consumer to report that there's no errors happening.

     * [...]or if there are significant changes in
     * interpretation of existing events or fields.

Carving out "significant changes" seems rather odd.

Let's just say "any changes"? E.g. I don't think my 2d3491b117c (tr2:
log N parent process names on Linux, 2021-08-27) qualifies as
significant change, but why not bump the version even for that?

In that case it was part of a release where we bumped the version
anyway, but let's come up with some sane criteria.

What's "significant" is also entirely subjective. I think 2d3491b117c is
trivial in the context of the entirety of our trace2 data. But if all
you care about is logging process trees it's highly significant.

     * Smaller changes, such as adding
     * a new field to an existing event, do not require an increment to the EVENT
     * format version.

I'd think a new sub-field would be in the same class as a new top-level
event field, i.e. adding a version wouldn't add any information. It
falls under the same general rule that you can traverse future data with
your "version" in hand, and a whitelist of fields you know how to
handle. If the version is the same but you've got new unknown fields you
shouldn't need to care.

IOW I think this would make more sense as a version bumping criteria:

    The version should be incremented whenever an existing consumer of
    trace2 data might want to act differently based on the new data.

    An exception to this is that any new event types do not merit
    bumping the version number. E.g. we have a top-level event type
    "error" now, but might hypothetically add a new "warning" type.
    Such an addition won't require bumping the version.

    Likewise adding new mandatory fields to existing events doesn't
    require bumping the version. E.g. the "error" type has (as of
    writing) a "fmt" and "msg" field. Let's say a future version adds an
    "id" (as in unique id for the error) field, such an addition won't
    require bumping the version.

    In other words, consumers of the trace2 JSON format are expected to
    walk the structure and only pick those things that they know about.
    Any unknown fields the consumer doesn't know about can be safely
    discarded. This won't apply if the version is bumped, then all bets
    are off, and the meaning of existing fields may or may not have
    changed.

    The idea is to encourage additive changes over changes to existing
    fields, and to reduce the work in maintaining the consumers of the
    format.

    As long as consumers ignore new unknown data they won't need to
    be updated every time the format changes in any way, only for
    potentially backwards-incompatible changes.

Wouldn't this be a saner policy for version bumping? AFAICT the only
thing you wouldn't be getting that you're getting now is the trivial
optimization of being able to say cheaply route trace2 payloads based on
version number alone (but even that is iffy now due to the subjectivity
of "significant change").

Re: [PATCH] trace2: increment event format version

[Josh Steadmon]

On 2021.12.01 16:57, Ævar Arnfjörð Bjarmason wrote:
> [...]
>
> IOW I think this would make more sense as a version bumping criteria:
> 
>     The version should be incremented whenever an existing consumer of
>     trace2 data might want to act differently based on the new data.
> 
>     An exception to this is that any new event types do not merit
>     bumping the version number. E.g. we have a top-level event type
>     "error" now, but might hypothetically add a new "warning" type.
>     Such an addition won't require bumping the version.
> 
>     Likewise adding new mandatory fields to existing events doesn't
>     require bumping the version. E.g. the "error" type has (as of
>     writing) a "fmt" and "msg" field. Let's say a future version adds an
>     "id" (as in unique id for the error) field, such an addition won't
>     require bumping the version.
> 
>     In other words, consumers of the trace2 JSON format are expected to
>     walk the structure and only pick those things that they know about.
>     Any unknown fields the consumer doesn't know about can be safely
>     discarded. This won't apply if the version is bumped, then all bets
>     are off, and the meaning of existing fields may or may not have
>     changed.
> 
>     The idea is to encourage additive changes over changes to existing
>     fields, and to reduce the work in maintaining the consumers of the
>     format.
> 
>     As long as consumers ignore new unknown data they won't need to
>     be updated every time the format changes in any way, only for
>     potentially backwards-incompatible changes.
> 
> Wouldn't this be a saner policy for version bumping? AFAICT the only
> thing you wouldn't be getting that you're getting now is the trivial
> optimization of being able to say cheaply route trace2 payloads based on
> version number alone (but even that is iffy now due to the subjectivity
> of "significant change").

No objections from me, this sounds like a good improvement.