Closed Bug 1557329 Opened 6 years ago Closed 6 years ago

Do we need to check shouldRecord twice for every metric operation?

Categories

(Data Platform and Tools :: Glean: SDK, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: chutten, Unassigned)

References

Details

(Whiteboard: [telemetry:glean-rs:m8])

To add to a CounterMetricType in Kotlin we end up doing the following

public fun add
private fun shouldRecord
FFI/glean_counter_should_record
pub trait MetricType fn should_record
Dispatch
FFI/glean_counter_add
pub fn CounterMetric.add
pub trait MetricType fn should_record
<and then the addition>

pub trait MetricType fn should_record is repeated, and we end up crossing the FFI boundary twice to do it. Can we avoid any/all of that?

I naively posited we could get rid of the kotlin-side shouldRecord altogether (it removes some FFI surface as well, which might be desirable), but Alessio pointed out that the Dispatch might be the most important part of the equation: we might want to preserve the behaviour where we don't dispatch if we shouldn't record.

Also, I said "crossing the FFI boundary twice" as though the two crossings were equivalent. In the case of, e.g. glean_string_list_set we have to convert a List<String> for FFI. The present arrangement allows us to avoid that conversion in cases when we shouldn't record.

Oops, missed an FFI boundary. kt/shouldRecord also calls FFI/glean_is_initialized

Looks like bug 1557339 is looking in to whether we need that third one at all, which may change the calculus here.

See Also: → 1557339
Whiteboard: [telemetry:glean-rs:m?] → [telemetry:glean-rs:m6]
Whiteboard: [telemetry:glean-rs:m6] → [telemetry:glean-rs:m8]

We changed how the Kotlin side works since then and only do a disabled check on the Kotlin side and do should record checks on the Rust side.
This bug is therefore invalid now.

Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.