fix(frames-delay): Update meta resolver frames delay units

[narsaynorath]

Map the mobile.frames_delay metric to seconds. Supposedly this can be retrieved from the MRI but I wasn't able to find the code path that did that, instead I found this meta unit resolver and added the mapping there.

It's similar to #81884 which adds the proper type for HTTP response sizes, but I didn't want to just add another property to the class.

Fixes #81560

[markushi]

Awesome, thank you! I did some more digging and the root cause for this issue seems to stem from the SpanMetricsDataSetConfig, as it defines a result_type_fn for functions like avg_if() to look up the type of the actual metric.

sentry/src/sentry/search/events/datasets/spans_metrics.py

Lines 256 to 276 in 62e12db

	fields.MetricsFunction(
	"avg_if",
	required_args=[
	fields.MetricArg(
	"column",
	allowed_columns=constants.SPAN_METRIC_DURATION_COLUMNS
	| constants.SPAN_METRIC_COUNT_COLUMNS,
	),
	fields.MetricArg(
	"if_col",
	allowed_columns=["release", "span.op"],
	),
	fields.SnQLStringArg(
	"if_val", unquote=True, unescape_quotes=True, optional_unquote=True
	),
	],
	calculated_args=[resolve_metric_id],
	snql_gauge=self._resolve_avg_if,
	snql_distribution=self._resolve_avg_if,
	result_type_fn=self.reflective_result_type(),
	default_result_type="duration",

But unless defined (that's what you did in the this PR), it simply calls MetricArg.get_type():

sentry/src/sentry/search/events/datasets/base.py

Lines 58 to 66 in cb8bba3

	def result_type_fn(
	function_arguments: list[fields.FunctionArg], parameter_values: dict[str, Any]
	) -> str:
	argument = function_arguments[index]
	value = parameter_values[argument.name]
	if (field_type := self.builder.get_field_type(value)) is not None: # type: ignore[attr-defined]
	return field_type
	else:
	return argument.get_type(value)

Which itself always defaults to number here:

sentry/src/sentry/search/events/fields.py

Lines 2234 to 2236 in cb8bba3

	def get_type(self, value: str) -> str:
	# Just a default
	return "number"

So I guess ideally the result_type_fn can properly look up it's known metrics and derive the unit from there? I gave it a quick try over here.

[narsaynorath]

So I guess ideally the result_type_fn can properly look up it's known metrics and derive the unit from there?

@markushi I like this solution better as it seems like the way it's intended to work (pull the unit from the MRI). Great find. Would you be able to merge it into these changes and take over the PR?

[narsaynorath]

Just some small comments. The changes look good to me and the tests are passing so I feel like this is a safe change to make. I can't approve my PR but I can tag someone to review it tomorrow.

[wmak]

Just checking, could we not use self.builder.get_field_type here instead? Or am I missing context 😬

[wmak]

Or rather, could we augment get_field_type to handle span metrics, we use that for search field typing as well so the units there need to be correct for the search syntax to handle units correctly. eg. if you search for some_span_metric:534k we need to know that its a number to know k means thousand

[narsaynorath]

hmm makes sense, I originally added a change there but I was updating the meta_resolver_map for this field specifically. @markushi I suppose we should port these changes over to that method. I can give that a crack since it's after hours for you right now.

I didn't realize this would only fix the units for the output type and not search

[narsaynorath]

@wmak Moved the changes over to the get_field_type method in SpansMetricsQueryBuilder. Let us know how you feel about it

https://github.com/getsentry/sentry/pull/82209/files#diff-4cf33582e331d118cce575a23d98881984f20b579edcba5122bbcb2a831f0d28R47

[wmak]

just double checking, the list of parsed_mri.units overlap the units that discover already uses?

[gggritso]

👀 nice fix! Should also fix the units for messaging.message.receive.latency 👍🏻

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #82209      +/-   ##
==========================================
- Coverage   87.49%   87.45%   -0.04%     
==========================================
  Files        9410     9394      -16     
  Lines      536846   536330     -516     
  Branches    21049    20971      -78     
==========================================
- Hits       469701   469062     -639     
- Misses      66776    66857      +81     
- Partials      369      411      +42     

[narsaynorath]

@wmak

just double checking, the list of parsed_mri.units overlap the units that discover already uses?

I just checked Relay, and there are a few metrics that provide units that are not supported under discover, but they're not relevant for the reported issue, i.e. usd is used as a unit in the AI modules and ratio is a defined as a unit for some of the web vitals calculated metrics.

I'm going to merge this since those cases will just return None as before, but if you feel like we should tackle those missing units then let me know and we can look at what makes sense for those missing units!