Move FileIO table labels into marker schemas

[fatadel]

Main | Deploy preview

Replace the hardcoded data.type branch that assembles FileIO table labels with a schema template. Optional segments let the template omit surrounding punctuation when source or filename fields are missing.

Closes #6295

---

Profile with IO enabled

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.88%. Comparing base (18e46d8) to head (08ae1f3).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6296      +/-   ##
==========================================
+ Coverage   83.84%   83.88%   +0.04%     
==========================================
  Files         352      353       +1     
  Lines       37807    37900      +93     
  Branches    10674    10691      +17     
==========================================
+ Hits        31698    31793      +95     
+ Misses       5679     5678       -1     
+ Partials      430      429       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[fatadel]

• When this is merged, create a bug for the Gecko side.

[canova]

Thanks for the PR! Before I review completely, I see that this PR does 2 main things:

• Adds optional segment syntax to marker labels
• Adds a tableLabel to FileIO markers

It's difficult to review these two changes when they are combined together. Can you please split them into 2 commits?

[fatadel]

Thanks for the PR! Before I review completely, I see that this PR does 2 main things:

* Adds optional segment syntax to marker labels

* Adds a tableLabel to FileIO markers

It's difficult to review these two changes when they are combined together. Can you please split them into 2 commits?

Done ✅

[canova]

Re the optional segmenting in labels using ``[[ ... ]]`:
I'm a bit unsure about adding more and more label templating features to the marker labels. It feels like it can get out of hand quickly and not that easy to get it right.
Pinging and @mstange and @fqueze in case they feel strongly about it.

One issue that I can see is ternaries and the optional segment conflicts right now. For example [[{marker.data.canceled ? 'cancelled' : 'ok'}]] never prints ok if I understand correctly.

On the other hand, I believe this current behavior of FileIO marker can be implemented using ternaries already, right? I assume it'll be longer, but it saves us from adding more complexity here. What do you think?

[fatadel]

Re the optional segmenting in labels using ``[[ ... ]]`: I'm a bit unsure about adding more and more label templating features to the marker labels. It feels like it can get out of hand quickly and not that easy to get it right. Pinging and @mstange and @fqueze in case they feel strongly about it.

One issue that I can see is ternaries and the optional segment conflicts right now. For example [[{marker.data.canceled ? 'cancelled' : 'ok'}]] never prints ok if I understand correctly.

On the other hand, I believe this current behavior of FileIO marker can be implemented using ternaries already, right? I assume it'll be longer, but it saves us from adding more complexity here. What do you think?

Your example does print ok when marker.data.canceled is false, optional segments are omitted only for missing, null, or empty-string values. But I agree that we are adding excessive complexity to the vocabulary.

We cannot have the same behavior with just the ternaries though. The current hardcoded FileIO code omits that entire section when source is empty. Existing ternaries cannot conditionally include the interpolation itself. So, instead of having '' we would have (empty).

[fatadel]

@canova Thanks for the review! I've addressed your other comments, however, I am not sure what to do with the increased vocabulary complexity...

[canova]

Thanks for thinking it through!

Re the optional segmenting in labels using ``[[ ... ]]: I'm a bit unsure about adding more and more label templating features to the marker labels. It feels like it can get out of hand quickly and not that easy to get it right. Pinging and @mstange and @fqueze in case they feel strongly about it. One issue that I can see is ternaries and the optional segment conflicts right now. For example [[{marker.data.canceled ? 'cancelled' : 'ok'}]]` never prints `ok` if I understand correctly.
On the other hand, I believe this current behavior of FileIO marker can be implemented using ternaries already, right? I assume it'll be longer, but it saves us from adding more complexity here. What do you think?

Your example does print ok when marker.data.canceled is false, optional segments are omitted only for missing, null, or empty-string values. But I agree that we are adding excessive complexity to the vocabulary.

Yeah I wasn't fully correct on "never" prints ok part. It prints ok on false but doesn't print ok when it's null, which I expect it to. Because I first expect the inner ternary to be evaluated and then the optional one. Here's an example profile

I have two markers, with canceled: null. And one marker schema has [[{marker.data.canceled ? 'cancelled' : 'ok'}]] and the other {marker.data.canceled ? 'cancelled' : 'ok'}. And I expect them both to show ok, but only the bare ternary one shows ok and not the null one.

We cannot have the same behavior with just the ternaries though. The current hardcoded FileIO code omits that entire section when source is empty. Existing ternaries cannot conditionally include the interpolation itself. So, instead of having '' we would have (empty).

You are right on that, but this sounds like a bug to me. We show (empty) while printing the tooltip to make it clear for the users, but it shouldn't be used while we are testing the condition inside the ternary or on the label. Note this isn't about the ternary condition, which already reads the raw payload value and treats '' as false. It's the plain {marker.data.x} interpolation that formats '' into (empty). I would prefer to fix that instead of working around it.

For example I think this if check should include checking empty strings too:

profiler/src/profile-logic/marker-schema.ts

Lines 266 to 268 in f8a4b7a

	if (value === undefined || value === null) {
	// This would return "undefined" or "null" otherwise.
	return '';

Once that's fixed the ternary version of this FileIO marker could be this:

{marker.data.source ? '(' : ''}{marker.data.source}{marker.data.source ? ') ' : ''}{marker.data.operation}{marker.data.filename ? ' — ' : ''}{marker.data.filename}

Not nice looking but good enough.

[fatadel]

Thanks for thinking it through!

Re the optional segmenting in labels using ``[[ ... ]]: I'm a bit unsure about adding more and more label templating features to the marker labels. It feels like it can get out of hand quickly and not that easy to get it right. Pinging and @mstange and @fqueze in case they feel strongly about it. One issue that I can see is ternaries and the optional segment conflicts right now. For example [[{marker.data.canceled ? 'cancelled' : 'ok'}]]` never prints `ok` if I understand correctly.
On the other hand, I believe this current behavior of FileIO marker can be implemented using ternaries already, right? I assume it'll be longer, but it saves us from adding more complexity here. What do you think?

Your example does print ok when marker.data.canceled is false, optional segments are omitted only for missing, null, or empty-string values. But I agree that we are adding excessive complexity to the vocabulary.

Yeah I wasn't fully correct on "never" prints ok part. It prints ok on false but doesn't print ok when it's null, which I expect it to. Because I first expect the inner ternary to be evaluated and then the optional one. Here's an example profile

I have two markers, with canceled: null. And one marker schema has [[{marker.data.canceled ? 'cancelled' : 'ok'}]] and the other {marker.data.canceled ? 'cancelled' : 'ok'}. And I expect them both to show ok, but only the bare ternary one shows ok and not the null one.

We cannot have the same behavior with just the ternaries though. The current hardcoded FileIO code omits that entire section when source is empty. Existing ternaries cannot conditionally include the interpolation itself. So, instead of having '' we would have (empty).

You are right on that, but this sounds like a bug to me. We show (empty) while printing the tooltip to make it clear for the users, but it shouldn't be used while we are testing the condition inside the ternary or on the label. Note this isn't about the ternary condition, which already reads the raw payload value and treats '' as false. It's the plain {marker.data.x} interpolation that formats '' into (empty). I would prefer to fix that instead of working around it.

For example I think this if check should include checking empty strings too:

profiler/src/profile-logic/marker-schema.ts

Lines 266 to 268 in f8a4b7a

	if (value === undefined || value === null) {
	// This would return "undefined" or "null" otherwise.
	return '';

Once that's fixed the ternary version of this FileIO marker could be this:

{marker.data.source ? '(' : ''}{marker.data.source}{marker.data.source ? ') ' : ''}{marker.data.operation}{marker.data.filename ? ' — ' : ''}{marker.data.filename}

Not nice looking but good enough.

Thanks for your response, @canova! That should indeed work. Implemented now.

[canova]

Thanks!

[fatadel]

• When this is merged, create a bug for the Gecko side.

https://bugzilla.mozilla.org/show_bug.cgi?id=2071892