Re: TRACE_IDs documentation? And potentially broken impl..?

[Gabriel Charette]

Tentatively trying benchmarking-dev@ per deprecation of other lists?

On Mon, Feb 12, 2018 at 5:44 PM Gabriel Charette <g...@google.com> wrote:

Hi,

I recently dug into the TRACE_ID_*() code while trying to fix toplevel.flow for TaskScheduler tasks.

The TaskAnnotator code uses TRACE_ID_MANGLE() which is documented as making a cross-process unique ID but I don't think it does...

TRACE_ID_MANGLE  is marked as deprecated in favor of TRACE_ID_{GLOBAL, LOCAL} but there only appears to be users of the LOCAL variant.. and I can't wrap my head around the documentation of what the difference even is between global and local?

In the impl, aside from the void* constructor (which merely reinterpret casts), TraceID::ForceMangle and TraceID::DontMangle are the exact same and I don't see logic to merge a process unique token in the ID..?

There is "prefix_" code that was added in https://codereview.chromium.org/2504753002 but it still appears to be unused a year later and the documentation appears to hint that one should inject the prefix manually?

I'm confused both about what the documentation says it does (for APIs that are documented) and by what the impl looks like it does.

Thanks for enlightening me!

Gab

[Primiano Tucci]

Ehsan is the right person for this, let's wait for him.

He made changes recently to the TRACE_ID logic so he is the most up to date source of truth.

IIRC the legacy TRACE_ID_MANGLE & friends were deprecated because they had an extremely odd semantic that violated any least surprise principle (the scope is different depending on the fact that the ID is a pointer or an int).

My understanding is that TRACE_ID_GLOBAL/LOCAL is the way to go, but let's wait for Ehsan to shed some light.

[Ehsan Chiniforooshan]

Hi Gab,

Below I try to answer some of your question. I'll take a look at your CL later to understand your usecase give more specific comments if needed.

TRACE_ID_MANGLE  is marked as deprecated in favor of TRACE_ID_{GLOBAL, LOCAL} but there only appears to be users of the LOCAL variant.. and I can't wrap my head around the documentation of what the difference even is between global and local?

TRACE_ID_LOCAL andTRACE_ID_GLOBAL macros change the flag value of  a TraceID, which results in different JSON outputs. The trace viewer will prepend the PID to local IDs so that they don't collide with IDs from other processes.

So, if you need an inter-process ID, e.g. open an async slice in process 1 and close it in process 2, you sould use a global ID:

process 1:

TRACE_EVENT_NESTABLE_ASYNC_BEGIN0("cat", "name",

    TRACE_ID_GLOBAL(event_id));

process 2:

TRACE_EVENT_NESTABLE_ASYNC_END0("cat", "name",

    TRACE_ID_GLOBAL(event_id));

If you have several async events in the same category you can add a scope string to make sure they are not mixed together:

TRACE_EVENT_NESTABLE_ASYNC_BEGIN0("cat", "name",

    TRACE_ID_WITH_SCOPE("scope_1", TRACE_ID_GLOBAL(event_id)));

Otherwise, if your events are all from the same process, use TRACE_ID_LOCAL to make sure they do not collide with events from other processes.

 There is "prefix_" code that was added in https://codereview.chromium.org/2504753002 but it still appears to be unused a year later and the documentation appears to hint that one should inject the prefix manually?

Yeah, sorry! A little bith of context: about a year and a half ago we had a project to make some trace macro's more explicit and more general for a project (network instrumentation) that later was cancelled due to some push backs. So, that's the reason. I have a low-priority task this quarter to clean-up unused/deprecated macros.

Composite IDs and linking IDs are the macros that were introduced last year but never used. If you have a good usecas for them, let me know to not delete them.

 

I'm confused both about what the documentation says it does (for APIs that are documented) and by what the impl looks like it does.

Hope this mail makes it less confusing. Let me know if you have any questions. If I got to do the clean up this quarter, I'll make sure to add more documentations along the way, too.

Ehsan

[Gabriel Charette]

Thanks! I see the difference now, ForceMangle and DontMangle look the same but when handed to TraceID hti different constructor overloads which set the |id_flags_| accordingly, which trace_log.cc then adapts as fits.

A cleanup would indeed be nice as this is fairly hard to grok.

On Mon, Feb 12, 2018 at 6:59 PM Ehsan Chiniforooshan <chinifo...@google.com> wrote:

Hi Gab,

Below I try to answer some of your question. I'll take a look at your CL later to understand your usecase give more specific comments if needed.

TRACE_ID_MANGLE  is marked as deprecated in favor of TRACE_ID_{GLOBAL, LOCAL} but there only appears to be users of the LOCAL variant.. and I can't wrap my head around the documentation of what the difference even is between global and local?

TRACE_ID_LOCAL andTRACE_ID_GLOBAL macros change the flag value of  a TraceID, which results in different JSON outputs. The trace viewer will prepend the PID to local IDs so that they don't collide with IDs from other processes.

So, if you need an inter-process ID, e.g. open an async slice in process 1 and close it in process 2, you sould use a global ID:

process 1:

TRACE_EVENT_NESTABLE_ASYNC_BEGIN0("cat", "name",

    TRACE_ID_GLOBAL(event_id));

process 2:

TRACE_EVENT_NESTABLE_ASYNC_END0("cat", "name",

    TRACE_ID_GLOBAL(event_id));

If you have several async events in the same category you can add a scope string to make sure they are not mixed together:

TRACE_EVENT_NESTABLE_ASYNC_BEGIN0("cat", "name",

    TRACE_ID_WITH_SCOPE("scope_1", TRACE_ID_GLOBAL(event_id)));

Otherwise, if your events are all from the same process, use TRACE_ID_LOCAL to make sure they do not collide with events from other processes.

Ah ah! This explanation makes it much clearer. Can you add this as-is to trace_event.h? It would have saved me hours of grokking yesterday..!

As for me thinking process ID mangling wasn't working correctly, it might have been an issue with an intermediate patch of mine as it seems to be fine with the CL I ended up submitting.

Thanks!