Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2504753002:
tracing: Introduce API for composite IDs (Closed)
|
Created:
4 years, 1 month ago by chiniforooshan Modified:
3 years, 11 months ago CC:
chromium-reviews, tracing+reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiontracing: Introduce API for composite IDs
Creating a GUID that fits into a 64-bit number can be tricky.
TRACE_ID_COMPOSITE would let the caller specify a sequence of
numbers as an ID to make GUID generation easier. This is the last
part of unifying trace IDs, previous parts being TRACE_ID_LOCAL
and TRACE_ID_GLOBAL macros and TRACE_LINK_IDS macro.
In this patch, the macro is not implemented in a very general
way:
1- It supports combining two numbers to make an ID. This should
be enough for current use cases in which a local ID is prefixed
with the PID or the tracing-PID to become a GUID. We can support
IDs consisting of more than two number when it is needed.
2- Since we do not want to increase the size of the TraceEvent
structure, we only support these composite IDs as the last
argument of the TRACE_LINK_IDS macro in which the ID is written
in args. For other use-cases, one can use a process-local ID and
link it to a GUID.
Example usage:
TRACE_LINK_IDS("cat", "name", 0x1000,
TRACE_ID_WITH_SCOPE("bla", 0x2000, 0x3000));
Generated JSON:
("ph":"=", ..., "id":"0x1000",
"args":{"linked_id":{"scope": "bla", "id2":["0x2000", "0x3000"]}})
Design Doc:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw7s/edit#heading=h.rgbjbsfu6a9s
BUG=catapult:#2465
Review-Url: https://codereview.chromium.org/2504753002
Cr-Commit-Position: refs/heads/master@{#444092}
Committed: https://chromium.googlesource.com/chromium/src/+/02ab37fa0143f56ae73f1a567b585ffbec5afcaf
Patch Set 1 #Patch Set 2 : cl format #Patch Set 3 : Slightly more efficient implementation #
Total comments: 8
Patch Set 4 : comments #Patch Set 5 : comments #Patch Set 6 : . #Patch Set 7 : comments #
Total comments: 4
Patch Set 8 : comments #
Messages
Total messages: 35 (21 generated)
The CQ bit was checked by chiniforooshan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
chiniforooshan@chromium.org changed reviewers: + caseq@chromium.org
Hi Andrey, Does this look OK to you?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Description was changed from
==========
Composite IDs
Creating a GUID that fits into a 64-bit number can be tricky.
TRACE_ID_COMPOSITE would let the caller specify a sequence of
numbers as an ID to make GUID generation easier. This is the last
part of unifying trace IDs, previous parts being TRACE_ID_LOCLA
and TRACE_ID_GLOBAL macros and TRACE_LINK_IDS macro.
In this patch, the macro is not implemented in a very general
way:
1- It supports combining two numbers to make an ID. This should
be enough for current use cases in which a local ID is prefixed
with the PID or the tracing-PID to become a GUID. We can support
IDs consisting of more than two number when it is needed.
2- Since we do not want to increase the size of the trace_event
structure, we only support these composite IDs as the last
argument of the TRACE_LINK_IDS macro in which the ID is written
in args. For other use-cases, one can use a process-local ID and
link it to a GUID.
Example usage:
TRACE_LINK_IDS("cat", "name", 0x1000,
TRACE_ID_COMPOSITE(0x2000, 0x3000));
Generated JSON:
("ph":"=", ..., "id":"0x1000",
"args":{"linked_id":{"id":["0x2000", "0x3000"]}})
BUG=
==========
to
==========
Composite IDs
Creating a GUID that fits into a 64-bit number can be tricky.
TRACE_ID_COMPOSITE would let the caller specify a sequence of
numbers as an ID to make GUID generation easier. This is the last
part of unifying trace IDs, previous parts being TRACE_ID_LOCLA
and TRACE_ID_GLOBAL macros and TRACE_LINK_IDS macro.
In this patch, the macro is not implemented in a very general
way:
1- It supports combining two numbers to make an ID. This should
be enough for current use cases in which a local ID is prefixed
with the PID or the tracing-PID to become a GUID. We can support
IDs consisting of more than two number when it is needed.
2- Since we do not want to increase the size of the trace_event
structure, we only support these composite IDs as the last
argument of the TRACE_LINK_IDS macro in which the ID is written
in args. For other use-cases, one can use a process-local ID and
link it to a GUID.
Example usage:
TRACE_LINK_IDS("cat", "name", 0x1000,
TRACE_ID_COMPOSITE(0x2000, 0x3000));
Generated JSON:
("ph":"=", ..., "id":"0x1000",
"args":{"linked_id":{"id":["0x2000", "0x3000"]}})
Design Doc:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw...
BUG=catapult:#2465
==========
Description was changed from
==========
Composite IDs
Creating a GUID that fits into a 64-bit number can be tricky.
TRACE_ID_COMPOSITE would let the caller specify a sequence of
numbers as an ID to make GUID generation easier. This is the last
part of unifying trace IDs, previous parts being TRACE_ID_LOCLA
and TRACE_ID_GLOBAL macros and TRACE_LINK_IDS macro.
In this patch, the macro is not implemented in a very general
way:
1- It supports combining two numbers to make an ID. This should
be enough for current use cases in which a local ID is prefixed
with the PID or the tracing-PID to become a GUID. We can support
IDs consisting of more than two number when it is needed.
2- Since we do not want to increase the size of the trace_event
structure, we only support these composite IDs as the last
argument of the TRACE_LINK_IDS macro in which the ID is written
in args. For other use-cases, one can use a process-local ID and
link it to a GUID.
Example usage:
TRACE_LINK_IDS("cat", "name", 0x1000,
TRACE_ID_COMPOSITE(0x2000, 0x3000));
Generated JSON:
("ph":"=", ..., "id":"0x1000",
"args":{"linked_id":{"id":["0x2000", "0x3000"]}})
Design Doc:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw...
BUG=catapult:#2465
==========
to
==========
Composite IDs
Creating a GUID that fits into a 64-bit number can be tricky.
TRACE_ID_COMPOSITE would let the caller specify a sequence of
numbers as an ID to make GUID generation easier. This is the last
part of unifying trace IDs, previous parts being TRACE_ID_LOCLA
and TRACE_ID_GLOBAL macros and TRACE_LINK_IDS macro.
In this patch, the macro is not implemented in a very general
way:
1- It supports combining two numbers to make an ID. This should
be enough for current use cases in which a local ID is prefixed
with the PID or the tracing-PID to become a GUID. We can support
IDs consisting of more than two number when it is needed.
2- Since we do not want to increase the size of the trace_event
structure, we only support these composite IDs as the last
argument of the TRACE_LINK_IDS macro in which the ID is written
in args. For other use-cases, one can use a process-local ID and
link it to a GUID.
Example usage:
TRACE_LINK_IDS("cat", "name", 0x1000,
TRACE_ID_COMPOSITE(0x2000, 0x3000));
Generated JSON:
("ph":"=", ..., "id":"0x1000",
"args":{"linked_id":{"id":["0x2000", "0x3000"]}})
Design Doc:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw...
BUG=catapult:#2465
==========
The CQ bit was checked by chiniforooshan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mostly looks good, though a bunch of nits. https://codereview.chromium.org/2504753002/diff/40001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/2504753002/diff/40001/base/trace_event/trace_... base/trace_event/trace_event.h:492: CompositeID& composite_id) { const ComositeID& https://codereview.chromium.org/2504753002/diff/40001/base/trace_event/trace_... base/trace_event/trace_event.h:498: static inline ScopedID MakeScopedID(const char* scope, T raw_id) { why do we need it to be template? looks like we only would pass the template parameter to a non-template ScopedID constructor that expects unsigned long long. https://codereview.chromium.org/2504753002/diff/40001/base/trace_event/trace_... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/2504753002/diff/40001/base/trace_event/trace_... base/trace_event/trace_event_impl.cc:462: std::string id_field_name = "id"; looks like this could be const char* instead. https://codereview.chromium.org/2504753002/diff/40001/base/trace_event/trace_... base/trace_event/trace_event_impl.cc:474: value->BeginArrayWithCopiedName(id_field_name); ... using const char* would let us get rid of coping here.
https://codereview.chromium.org/2504753002/diff/40001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/2504753002/diff/40001/base/trace_event/trace_... base/trace_event/trace_event.h:492: CompositeID& composite_id) { On 2016/11/21 19:47:20, caseq wrote: > const ComositeID& But composite_id is actually modified in this function. https://codereview.chromium.org/2504753002/diff/40001/base/trace_event/trace_... base/trace_event/trace_event.h:498: static inline ScopedID MakeScopedID(const char* scope, T raw_id) { On 2016/11/21 19:47:20, caseq wrote: > why do we need it to be template? looks like we only would pass the template > parameter to a non-template ScopedID constructor that expects unsigned long > long. I think you missed the other two constructors :) ScopedID constructor"s" expect unsgined long long, LocalID, or GlobalID. https://codereview.chromium.org/2504753002/diff/40001/base/trace_event/trace_... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/2504753002/diff/40001/base/trace_event/trace_... base/trace_event/trace_event_impl.cc:462: std::string id_field_name = "id"; On 2016/11/21 19:47:20, caseq wrote: > looks like this could be const char* instead. Done. https://codereview.chromium.org/2504753002/diff/40001/base/trace_event/trace_... base/trace_event/trace_event_impl.cc:474: value->BeginArrayWithCopiedName(id_field_name); On 2016/11/21 19:47:20, caseq wrote: > ... using const char* would let us get rid of coping here. Done.
On 2016/11/21 21:12:15, chiniforooshan wrote: > https://codereview.chromium.org/2504753002/diff/40001/base/trace_event/trace_... > File base/trace_event/trace_event.h (right): > > https://codereview.chromium.org/2504753002/diff/40001/base/trace_event/trace_... > base/trace_event/trace_event.h:492: CompositeID& composite_id) { > On 2016/11/21 19:47:20, caseq wrote: > > const ComositeID& > > But composite_id is actually modified in this function. Oh, indeed! On a second thought, do we ever need local id to be composite? I think it's not needed for our use case and might provoke some usages that we don't want, so perhaps drop it altogether? lgtm.
Description was changed from
==========
Composite IDs
Creating a GUID that fits into a 64-bit number can be tricky.
TRACE_ID_COMPOSITE would let the caller specify a sequence of
numbers as an ID to make GUID generation easier. This is the last
part of unifying trace IDs, previous parts being TRACE_ID_LOCLA
and TRACE_ID_GLOBAL macros and TRACE_LINK_IDS macro.
In this patch, the macro is not implemented in a very general
way:
1- It supports combining two numbers to make an ID. This should
be enough for current use cases in which a local ID is prefixed
with the PID or the tracing-PID to become a GUID. We can support
IDs consisting of more than two number when it is needed.
2- Since we do not want to increase the size of the trace_event
structure, we only support these composite IDs as the last
argument of the TRACE_LINK_IDS macro in which the ID is written
in args. For other use-cases, one can use a process-local ID and
link it to a GUID.
Example usage:
TRACE_LINK_IDS("cat", "name", 0x1000,
TRACE_ID_COMPOSITE(0x2000, 0x3000));
Generated JSON:
("ph":"=", ..., "id":"0x1000",
"args":{"linked_id":{"id":["0x2000", "0x3000"]}})
Design Doc:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw...
BUG=catapult:#2465
==========
to
==========
Composite IDs
Creating a GUID that fits into a 64-bit number can be tricky.
TRACE_ID_COMPOSITE would let the caller specify a sequence of
numbers as an ID to make GUID generation easier. This is the last
part of unifying trace IDs, previous parts being TRACE_ID_LOCAL
and TRACE_ID_GLOBAL macros and TRACE_LINK_IDS macro.
In this patch, the macro is not implemented in a very general
way:
1- It supports combining two numbers to make an ID. This should
be enough for current use cases in which a local ID is prefixed
with the PID or the tracing-PID to become a GUID. We can support
IDs consisting of more than two number when it is needed.
2- Since we do not want to increase the size of the TraceEvent
structure, we only support these composite IDs as the last
argument of the TRACE_LINK_IDS macro in which the ID is written
in args. For other use-cases, one can use a process-local ID and
link it to a GUID.
Example usage:
TRACE_LINK_IDS("cat", "name", 0x1000,
TRACE_ID_COMPOSITE(0x2000, 0x3000));
Generated JSON:
("ph":"=", ..., "id":"0x1000",
"args":{"linked_id":{"id":["0x2000", "0x3000"]}})
Design Doc:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw...
BUG=catapult:#2465
==========
On 2016/11/22 00:59:34, caseq wrote: > On 2016/11/21 21:12:15, chiniforooshan wrote: > > > https://codereview.chromium.org/2504753002/diff/40001/base/trace_event/trace_... > > File base/trace_event/trace_event.h (right): > > > > > https://codereview.chromium.org/2504753002/diff/40001/base/trace_event/trace_... > > base/trace_event/trace_event.h:492: CompositeID& composite_id) { > > On 2016/11/21 19:47:20, caseq wrote: > > > const ComositeID& > > > > But composite_id is actually modified in this function. > > Oh, indeed! On a second thought, do we ever need local id to be composite? I > think it's not needed for our use case and might provoke some usages that we > don't want, so perhaps drop it altogether? > > lgtm. Done. Thanks Andrey! Primiano, could you please take a look?
chiniforooshan@chromium.org changed reviewers: + primiano@chromium.org
On 2016/11/22 16:04:06, chiniforooshan wrote: > On 2016/11/22 00:59:34, caseq wrote: > > On 2016/11/21 21:12:15, chiniforooshan wrote: > > > > > > https://codereview.chromium.org/2504753002/diff/40001/base/trace_event/trace_... > > > File base/trace_event/trace_event.h (right): > > > > > > > > > https://codereview.chromium.org/2504753002/diff/40001/base/trace_event/trace_... > > > base/trace_event/trace_event.h:492: CompositeID& composite_id) { > > > On 2016/11/21 19:47:20, caseq wrote: > > > > const ComositeID& > > > > > > But composite_id is actually modified in this function. > > > > Oh, indeed! On a second thought, do we ever need local id to be composite? I > > think it's not needed for our use case and might provoke some usages that we > > don't want, so perhaps drop it altogether? > > > > lgtm. > > Done. > > Thanks Andrey! > > Primiano, could you please take a look? Is there a chance to see how this would be really used in a real case? I am trying to get my head around this and figure out why the bind-id macros themselves are not enough, but is hard to do in absence of any use case. It seem you are saying here: regardless of the scope, which now can be also global, the current ID system is not powerful enough? Is it that the case? why?
We had a chat offline about his during the PI meeting. My only question here is: can this problem be solved by adding another int field (or two) to the existing "class WithScope"? My understanding is that from a conceptual viewpoint we could use the scope string to solve the problem without adding anything else. However this would suck from a performance and client code viewpoint because will requires some sprintf. So my question is: if we say that a scope can be (string, optional int), where the semantic is the same of doing a sprintf, without actually doing it (i.e. two scope are considered identical if both parts are identical), does it solve the problem? If the answer is yes, I'd prefer that we just add a field to WithScope. If not let's do your way. just please add an example to show how this is used in the code so this CL gets more context when we'll look back to it.
On 2016/12/01 18:49:04, Primiano Tucci wrote: > We had a chat offline about his during the PI meeting. > My only question here is: can this problem be solved by adding another int field > (or two) to the existing "class WithScope"? > My understanding is that from a conceptual viewpoint we could use the scope > string to solve the problem without adding anything else. However this would > suck from a performance and client code viewpoint because will requires some > sprintf. > So my question is: if we say that a scope can be (string, optional int), where > the semantic is the same of doing a sprintf, without actually doing it (i.e. two > scope are considered identical if both parts are identical), does it solve the > problem? > If the answer is yes, I'd prefer that we just add a field to WithScope. > If not let's do your way. > just please add an example to show how this is used in the code so this CL gets > more context when we'll look back to it. I think it is possible to bundle it with the scope string. But I don't understand why that's preferable. So, we are in a situation that we need IDs of the form (string, int, int). It seems nicer to call the string part one thing (e.g. scope) and the int parts another thing (e.g. composite ID). Implementation-wise, we still have build a TracedValue, anyways. The current implementation fits more naturally with the future proto-based events, too: we just have to make the ID field a repeated field. If you don't have a strong preference for making the PID/TPID part of the scope, I would like to keep this implementation.
On 2016/12/05 21:41:40, chiniforooshan1 wrote:
> I think it is possible to bundle it with the scope string. But I don't
> understand why that's preferable.
Because I'm trying to see this with the eyes of somebody who approaches the
tracing codebase for the first time and sees:
TRACE_ID_WITH_SCOPE(scope, id)
TRACE_ID_GLOBAL(id)
TRACE_ID_LOCAL(id)
TRACE_ID_COMPOSITE(prefix, id)
and tries to figures out how to use them.
The alternative I'm proposing here is
TRACE_ID([GLOBAL | LOCAL], scope, arg1, arg2)
So you can just use as follows:
TRACE_ID(GLOBAL, "/foo/bar/", number1, number2)
which is almost as readable as a printf. No sprintf needs to happen on the
flight, you just keep them as separate fields internally.
Eventually if you want you could strcat them at serialization time, so the JSON
becomes {ph:'...', ...., id2:'global:/foo/bar/numer1/number2'}
which would make also the importer easier to read and develope because that
would just do plain string matching %global/local?
(but maybe the importer ship has sailed already in which case ignore this)
> So, we are in a situation that we need IDs of the form (string, int, int). It
> seems nicer to call the string part one thing (e.g. scope) and the int parts
> another thing (e.g. composite ID).
My view is that you want to ultimately expose is a simple semantic, that is: two
IDs match if all their parts match (% global vs local).
IMHO something that looks like a printf format (even if, again, it does *not*
need to be a real printf under the hoods) is just simple and readable.
Instead you here want people to understand how to plug together these four extra
macros, when you could have only one.
> Implementation-wise, we still have build a TracedValue, anyways. The current
> implementation fits more naturally with the future proto-based events, too: we
> just have to make the ID field a repeated field.
Both of them really won't make any difference. It's all about the macro we
expose to the rest of the codebase, not the TracedValue.
> If you don't have a strong preference for making the PID/TPID part of the
scope,
> I would like to keep this implementation.
I just have a preference for exposing APIs that are as simple as possible. If
something simple can do the job why do we need to expose something more complex?
Is there any advantage I'm missing?
On 2016/12/06 18:52:53, Primiano Tucci (back but slow) wrote:
> On 2016/12/05 21:41:40, chiniforooshan1 wrote:
> > I think it is possible to bundle it with the scope string. But I don't
> > understand why that's preferable.
>
> Because I'm trying to see this with the eyes of somebody who approaches the
> tracing codebase for the first time and sees:
>
> TRACE_ID_WITH_SCOPE(scope, id)
> TRACE_ID_GLOBAL(id)
> TRACE_ID_LOCAL(id)
> TRACE_ID_COMPOSITE(prefix, id)
>
> and tries to figures out how to use them.
>
> The alternative I'm proposing here is
> TRACE_ID([GLOBAL | LOCAL], scope, arg1, arg2)
>
> So you can just use as follows:
> TRACE_ID(GLOBAL, "/foo/bar/", number1, number2)
>
> which is almost as readable as a printf. No sprintf needs to happen on the
> flight, you just keep them as separate fields internally.
>
> Eventually if you want you could strcat them at serialization time, so the
JSON
> becomes {ph:'...', ...., id2:'global:/foo/bar/numer1/number2'}
> which would make also the importer easier to read and develope because that
> would just do plain string matching %global/local?
> (but maybe the importer ship has sailed already in which case ignore this)
>
> > So, we are in a situation that we need IDs of the form (string, int, int).
It
> > seems nicer to call the string part one thing (e.g. scope) and the int parts
> > another thing (e.g. composite ID).
>
> My view is that you want to ultimately expose is a simple semantic, that is:
two
> IDs match if all their parts match (% global vs local).
> IMHO something that looks like a printf format (even if, again, it does *not*
> need to be a real printf under the hoods) is just simple and readable.
> Instead you here want people to understand how to plug together these four
extra
> macros, when you could have only one.
>
> > Implementation-wise, we still have build a TracedValue, anyways. The current
> > implementation fits more naturally with the future proto-based events, too:
we
> > just have to make the ID field a repeated field.
>
> Both of them really won't make any difference. It's all about the macro we
> expose to the rest of the codebase, not the TracedValue.
>
> > If you don't have a strong preference for making the PID/TPID part of the
> scope,
> > I would like to keep this implementation.
> I just have a preference for exposing APIs that are as simple as possible. If
> something simple can do the job why do we need to expose something more
complex?
> Is there any advantage I'm missing?
Done. Changed the macros according to your suggestion. PTAL. Thanks!
Description was changed from
==========
Composite IDs
Creating a GUID that fits into a 64-bit number can be tricky.
TRACE_ID_COMPOSITE would let the caller specify a sequence of
numbers as an ID to make GUID generation easier. This is the last
part of unifying trace IDs, previous parts being TRACE_ID_LOCAL
and TRACE_ID_GLOBAL macros and TRACE_LINK_IDS macro.
In this patch, the macro is not implemented in a very general
way:
1- It supports combining two numbers to make an ID. This should
be enough for current use cases in which a local ID is prefixed
with the PID or the tracing-PID to become a GUID. We can support
IDs consisting of more than two number when it is needed.
2- Since we do not want to increase the size of the TraceEvent
structure, we only support these composite IDs as the last
argument of the TRACE_LINK_IDS macro in which the ID is written
in args. For other use-cases, one can use a process-local ID and
link it to a GUID.
Example usage:
TRACE_LINK_IDS("cat", "name", 0x1000,
TRACE_ID_COMPOSITE(0x2000, 0x3000));
Generated JSON:
("ph":"=", ..., "id":"0x1000",
"args":{"linked_id":{"id":["0x2000", "0x3000"]}})
Design Doc:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw...
BUG=catapult:#2465
==========
to
==========
Composite IDs
Creating a GUID that fits into a 64-bit number can be tricky.
TRACE_ID_COMPOSITE would let the caller specify a sequence of
numbers as an ID to make GUID generation easier. This is the last
part of unifying trace IDs, previous parts being TRACE_ID_LOCAL
and TRACE_ID_GLOBAL macros and TRACE_LINK_IDS macro.
In this patch, the macro is not implemented in a very general
way:
1- It supports combining two numbers to make an ID. This should
be enough for current use cases in which a local ID is prefixed
with the PID or the tracing-PID to become a GUID. We can support
IDs consisting of more than two number when it is needed.
2- Since we do not want to increase the size of the TraceEvent
structure, we only support these composite IDs as the last
argument of the TRACE_LINK_IDS macro in which the ID is written
in args. For other use-cases, one can use a process-local ID and
link it to a GUID.
Example usage:
TRACE_LINK_IDS("cat", "name", 0x1000,
TRACE_ID_WITH_SCOPE("bla", 0x2000, 0x3000));
Generated JSON:
("ph":"=", ..., "id":"0x1000",
"args":{"linked_id":{"id":["0x2000", "0x3000"]}})
Design Doc:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw...
BUG=catapult:#2465
==========
Description was changed from
==========
Composite IDs
Creating a GUID that fits into a 64-bit number can be tricky.
TRACE_ID_COMPOSITE would let the caller specify a sequence of
numbers as an ID to make GUID generation easier. This is the last
part of unifying trace IDs, previous parts being TRACE_ID_LOCAL
and TRACE_ID_GLOBAL macros and TRACE_LINK_IDS macro.
In this patch, the macro is not implemented in a very general
way:
1- It supports combining two numbers to make an ID. This should
be enough for current use cases in which a local ID is prefixed
with the PID or the tracing-PID to become a GUID. We can support
IDs consisting of more than two number when it is needed.
2- Since we do not want to increase the size of the TraceEvent
structure, we only support these composite IDs as the last
argument of the TRACE_LINK_IDS macro in which the ID is written
in args. For other use-cases, one can use a process-local ID and
link it to a GUID.
Example usage:
TRACE_LINK_IDS("cat", "name", 0x1000,
TRACE_ID_WITH_SCOPE("bla", 0x2000, 0x3000));
Generated JSON:
("ph":"=", ..., "id":"0x1000",
"args":{"linked_id":{"id":["0x2000", "0x3000"]}})
Design Doc:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw...
BUG=catapult:#2465
==========
to
==========
Composite IDs
Creating a GUID that fits into a 64-bit number can be tricky.
TRACE_ID_COMPOSITE would let the caller specify a sequence of
numbers as an ID to make GUID generation easier. This is the last
part of unifying trace IDs, previous parts being TRACE_ID_LOCAL
and TRACE_ID_GLOBAL macros and TRACE_LINK_IDS macro.
In this patch, the macro is not implemented in a very general
way:
1- It supports combining two numbers to make an ID. This should
be enough for current use cases in which a local ID is prefixed
with the PID or the tracing-PID to become a GUID. We can support
IDs consisting of more than two number when it is needed.
2- Since we do not want to increase the size of the TraceEvent
structure, we only support these composite IDs as the last
argument of the TRACE_LINK_IDS macro in which the ID is written
in args. For other use-cases, one can use a process-local ID and
link it to a GUID.
Example usage:
TRACE_LINK_IDS("cat", "name", 0x1000,
TRACE_ID_WITH_SCOPE("bla", 0x2000, 0x3000));
Generated JSON:
("ph":"=", ..., "id":"0x1000",
"args":{"linked_id":{"scope": "bla", "id2":["0x2000", "0x3000"]}})
Design Doc:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw...
BUG=catapult:#2465
==========
LGTM with a small comment. Also plz update the cl title with something a bit more descriptive, like: tracing: introduce API for composite IDs https://codereview.chromium.org/2504753002/diff/120001/base/trace_event/trace... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/2504753002/diff/120001/base/trace_event/trace... base/trace_event/trace_event.h:46: // this macro to add a scope string. can you just add some examples in the comment to show how this would be used (one for the raw and one with the id?) https://codereview.chromium.org/2504753002/diff/120001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/2504753002/diff/120001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:474: value->BeginArray(id_field_name); by looking at the else case below this value can be either a string or an array, which might be confusing as a the trace format. wouldn't be easier for everybody if you just concatenate these into a string, instead of creating an array, e.g.: "$prefix/$raw_id" ?
Description was changed from
==========
Composite IDs
Creating a GUID that fits into a 64-bit number can be tricky.
TRACE_ID_COMPOSITE would let the caller specify a sequence of
numbers as an ID to make GUID generation easier. This is the last
part of unifying trace IDs, previous parts being TRACE_ID_LOCAL
and TRACE_ID_GLOBAL macros and TRACE_LINK_IDS macro.
In this patch, the macro is not implemented in a very general
way:
1- It supports combining two numbers to make an ID. This should
be enough for current use cases in which a local ID is prefixed
with the PID or the tracing-PID to become a GUID. We can support
IDs consisting of more than two number when it is needed.
2- Since we do not want to increase the size of the TraceEvent
structure, we only support these composite IDs as the last
argument of the TRACE_LINK_IDS macro in which the ID is written
in args. For other use-cases, one can use a process-local ID and
link it to a GUID.
Example usage:
TRACE_LINK_IDS("cat", "name", 0x1000,
TRACE_ID_WITH_SCOPE("bla", 0x2000, 0x3000));
Generated JSON:
("ph":"=", ..., "id":"0x1000",
"args":{"linked_id":{"scope": "bla", "id2":["0x2000", "0x3000"]}})
Design Doc:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw...
BUG=catapult:#2465
==========
to
==========
tracing: Introduce API for composite IDs
Creating a GUID that fits into a 64-bit number can be tricky.
TRACE_ID_COMPOSITE would let the caller specify a sequence of
numbers as an ID to make GUID generation easier. This is the last
part of unifying trace IDs, previous parts being TRACE_ID_LOCAL
and TRACE_ID_GLOBAL macros and TRACE_LINK_IDS macro.
In this patch, the macro is not implemented in a very general
way:
1- It supports combining two numbers to make an ID. This should
be enough for current use cases in which a local ID is prefixed
with the PID or the tracing-PID to become a GUID. We can support
IDs consisting of more than two number when it is needed.
2- Since we do not want to increase the size of the TraceEvent
structure, we only support these composite IDs as the last
argument of the TRACE_LINK_IDS macro in which the ID is written
in args. For other use-cases, one can use a process-local ID and
link it to a GUID.
Example usage:
TRACE_LINK_IDS("cat", "name", 0x1000,
TRACE_ID_WITH_SCOPE("bla", 0x2000, 0x3000));
Generated JSON:
("ph":"=", ..., "id":"0x1000",
"args":{"linked_id":{"scope": "bla", "id2":["0x2000", "0x3000"]}})
Design Doc:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw...
BUG=catapult:#2465
==========
chiniforooshan@google.com changed reviewers: + chiniforooshan@google.com
https://codereview.chromium.org/2504753002/diff/120001/base/trace_event/trace... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/2504753002/diff/120001/base/trace_event/trace... base/trace_event/trace_event.h:46: // this macro to add a scope string. On 2017/01/17 12:25:47, Primiano Tucci wrote: > can you just add some examples in the comment to show how this would be used > (one for the raw and one with the id?) Done. https://codereview.chromium.org/2504753002/diff/120001/base/trace_event/trace... File base/trace_event/trace_event_impl.cc (right): https://codereview.chromium.org/2504753002/diff/120001/base/trace_event/trace... base/trace_event/trace_event_impl.cc:474: value->BeginArray(id_field_name); On 2017/01/17 12:25:47, Primiano Tucci wrote: > by looking at the else case below this value can be either a string or an array, > which might be confusing as a the trace format. > wouldn't be easier for everybody if you just concatenate these into a string, > instead of creating an array, e.g.: > "$prefix/$raw_id" ? > Done.
The CQ bit was checked by chiniforooshan@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2504753002/#ps140001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484671517532480,
"parent_rev": "f9f7a98a1cc5e576b47d9be3476304e5f12562d1", "commit_rev":
"02ab37fa0143f56ae73f1a567b585ffbec5afcaf"}
Message was sent while issue was closed.
Description was changed from
==========
tracing: Introduce API for composite IDs
Creating a GUID that fits into a 64-bit number can be tricky.
TRACE_ID_COMPOSITE would let the caller specify a sequence of
numbers as an ID to make GUID generation easier. This is the last
part of unifying trace IDs, previous parts being TRACE_ID_LOCAL
and TRACE_ID_GLOBAL macros and TRACE_LINK_IDS macro.
In this patch, the macro is not implemented in a very general
way:
1- It supports combining two numbers to make an ID. This should
be enough for current use cases in which a local ID is prefixed
with the PID or the tracing-PID to become a GUID. We can support
IDs consisting of more than two number when it is needed.
2- Since we do not want to increase the size of the TraceEvent
structure, we only support these composite IDs as the last
argument of the TRACE_LINK_IDS macro in which the ID is written
in args. For other use-cases, one can use a process-local ID and
link it to a GUID.
Example usage:
TRACE_LINK_IDS("cat", "name", 0x1000,
TRACE_ID_WITH_SCOPE("bla", 0x2000, 0x3000));
Generated JSON:
("ph":"=", ..., "id":"0x1000",
"args":{"linked_id":{"scope": "bla", "id2":["0x2000", "0x3000"]}})
Design Doc:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw...
BUG=catapult:#2465
==========
to
==========
tracing: Introduce API for composite IDs
Creating a GUID that fits into a 64-bit number can be tricky.
TRACE_ID_COMPOSITE would let the caller specify a sequence of
numbers as an ID to make GUID generation easier. This is the last
part of unifying trace IDs, previous parts being TRACE_ID_LOCAL
and TRACE_ID_GLOBAL macros and TRACE_LINK_IDS macro.
In this patch, the macro is not implemented in a very general
way:
1- It supports combining two numbers to make an ID. This should
be enough for current use cases in which a local ID is prefixed
with the PID or the tracing-PID to become a GUID. We can support
IDs consisting of more than two number when it is needed.
2- Since we do not want to increase the size of the TraceEvent
structure, we only support these composite IDs as the last
argument of the TRACE_LINK_IDS macro in which the ID is written
in args. For other use-cases, one can use a process-local ID and
link it to a GUID.
Example usage:
TRACE_LINK_IDS("cat", "name", 0x1000,
TRACE_ID_WITH_SCOPE("bla", 0x2000, 0x3000));
Generated JSON:
("ph":"=", ..., "id":"0x1000",
"args":{"linked_id":{"scope": "bla", "id2":["0x2000", "0x3000"]}})
Design Doc:
https://docs.google.com/document/d/1s0DKjNJk85hDuRp5IqujwZvPML-MPsyAtDeksMwBw...
BUG=catapult:#2465
Review-Url: https://codereview.chromium.org/2504753002
Cr-Commit-Position: refs/heads/master@{#444092}
Committed:
https://chromium.googlesource.com/chromium/src/+/02ab37fa0143f56ae73f1a567b58...
==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/02ab37fa0143f56ae73f1a567b58... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
