Tracing model fails if performance.measure names have ':'

[krisselden]

If you record a trace of https://www.linkedin.com/feed/?_ember-perf-timeline=true the trace will fail to load based on this regex matching https://github.com/catapult-project/catapult/commit/56ebb1bdfc67d48a5549f012edd56994a0cdbbfc#diff-0974038a331c3680e24f884c5980c30cR1064 a non base64 string and causing an Error to be thrown in Base64.atob

[fmeawad]

CC @lpy

[saurabhgoyaliitr]

Hi, is this issue still open?

[krisselden]

@saurabhgoyaliitr this is still an issue. @fmeawad it isn't a good first issue because it is very unclear why the code exists in the first place. The document that issue links to is private and I have no clue what convention it is trying to parse comes from.

I can make it so it catches and doesn't process it further, but it would be nice to understand what the convention actually. Is this code even needed still?

As far as I know, performance.mark() is just a string.

[krisselden]

OK, to clarify this issue better, user land code accidentally trips this parsing code which has no public reasoning why it exists (the google doc that explains why it was added is private).

// app instrumentation
performance.measure("<service:tracking>:tracking-event", "<service:tracking>:tracking-event-start", "<service:tracking>:tracking-event-end");

will generate a TraceEvent that looks like

{"pid":4819,"tid":775,"ts":2695560724486,"ph":"b","cat":"blink.user_timing","name":"\u003Cservice:tracking>:tracking-event","args":{},"tts":9750347,"id":"0x978b00"},
{"pid":4819,"tid":775,"ts":2695560724571,"ph":"e","cat":"blink.user_timing","name":"\u003Cservice:tracking>:tracking-event","args":{},"tts":9750355,"id":"0x978b00"},

What ever reason this code is trying to parse name for, at the moment the most distinguishing factor from the unit test is the unit test events have "args":{"params":""} maybe just checking for the "params" key will be enough to avoid conflict with user land measures.

[krisselden]

another way of asking is how does one replicate with an actual recorded trace, the output in the unit tests here? https://github.com/catapult-project/catapult/blob/56ebb1bdfc67d48a5549f012edd56994a0cdbbfc/tracing/tracing/extras/importer/trace_event_importer_test.html#L5137

[fmeawad]

One way to generate a similar trace file is to record a trace of trace importing (open chrome://tracing in a tab, hit record, open chrome://tracing in another tab, and open a recorded trace, once importing is done, go to original chrome://tracing tab and stop recording).

These events are based on adding grouping to Mark/Measure instrumentation instead of the tracing timing.html library (a tracing internal library to measure tracing operations performance)

The code was added in this CL

The reasoning was to create a hierarchical tracking of mark/measure events (instead of putting all of them in one group), as well as append the args in 64 bit encoding.

So in the example in the previous comment: This creates a group called "A", and a task "a1" and args of {params: ''}. It was done this way because the mark/measure API only allows providing the |name|.

This hierarchy was intended to be made public so any web developer that is using mark/measure + tracing can take advantage of the better visualization. I guess calling mark/measure with name "Group:Event" can be added to any code base, but the args part is the one that requires special processing like the one added in timing.html.

In the example in the earlier comment it does not seem to have a "/" separator in the name, so I am not sure why it is attempting to decode that part.

[krisselden]

@fmeawad we have urls in our measures that hit the slash.

This hierarchy was intended to be made public so any web developer that is using mark/measure + tracing can take advantage of the better visualization.

Where was this communicated out? This would be nice.

May I suggest though that open source commits not link private documents?

I know how to record a trace, I wasn't sure if this had to do with an internal thing or whether it was intended that it affected people using performance.measure() but you just answered my question.

There are several ways I could fix, one is to make it fall through on error. In addition to that I could narrow the RegEx. At least make the suffix stricter `(?:\/([A-Za-z0-9+/]+))?$

Thank you again for taking the time to reply you clarified my main inquiry with "This hierarchy was intended to be made public"

[krisselden]

I submitted a patch https://chromium-review.googlesource.com/c/catapult/+/1330263

[krisselden]

The review seems to have stalled, is there anything more I can do to help it along?