Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 1627841 - Cancel on exceptions in wrapped measurement function #808

Merged
merged 2 commits into from
Apr 8, 2020
Merged

Bug 1627841 - Cancel on exceptions in wrapped measurement function #808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a3968ad
Kotlin: Cancel on exceptions in wrapped measurement function
badboy Apr 7, 2020
6497c09
Swift: Add another convenience function to measure fallible functions
badboy Apr 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions glean-core/android/src/main/java/mozilla/telemetry/glean/private/TimespanMetricType.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,14 +90,22 @@ class TimespanMetricType internal constructor(

/**
* Convenience method to simplify measuring a function or block of code
*
* If the measured function throws, the measurement is canceled and the exception rethrown.
*/
@Suppress("TooGenericExceptionCaught")
inline fun <U> measure(funcToMeasure: () -> U): U {
start()
try {
return funcToMeasure()
} finally {
stop()

val returnValue = try {
funcToMeasure()
} catch (e: Exception) {
cancel()
throw e
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should throw exceptions from our APIs. We never do, our APIs are execption safe. We should instead document that we swallow exception and if a different behaviour is required then the product should not use this method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I kinda disagree with that. We're handling user-provided functions (for convenience) and we're merely passing on the behavior of their code (it's not that different from returning their return value).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be the only API function behaving different, which is something I don't think we should do (... that would be an exception ). We can handle user-defined function, but it doesn't mean it should break consistency with our other APIs.

This doesn't block any other use-case, since the other APIs can still be used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it doesn't behave "differently". Glean itself still doesn't produce any exceptions (I see what you did there!).

If users have potentially-throwing functions they need to measure they again need to come up with the exact same wrappper I wrote here instead of using our measure utility.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functions with exceptions can still be measured. To me this behaves differently: we guarantee that our APIs do not throw exceptions. Having one API suddenly behaving differently (even if the exception originates from a user defined function) can be confusing and a source of subtle bugs. Consumers of the API have two (with no exception) have 3 choices:

(1) Catch any exception in the function they need to measure.
(2) Live with the fact that our APIs swallow exceptions.
(3) Not use this utility wrapper.

I'd rather have consistency with the fact that users can expect no exception to come through our APIs than this added flexibility

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@travis79 I do like the idea of allowing choosing a behaviour, but I think we should always cancel on exception and only allow to choose whether or not to propagate the exception :)

As long as we default to propagating the exception since that would be the behavior if the function weren't being measured, I am okay with making that configurable within the function. As for whether or not to cancel? My opinion on that is weakly held as I don't have a concrete use case for not cancelling, so I am fine with the cancel on exception behavior also.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as we default to propagating the exception since that would be the behavior if the function weren't being measured

I think we should definitely cancel, but I'm not sure we should default to "propagate exceptions". See my previous comments :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should definitely cancel, but I'm not sure we should default to "propagate exceptions". See my previous comments :)

The only problem is that we would be swallowing application exceptions and not Glean exceptions, and I'm not comfortable with that. If a consuming application expected a function to throw in certain cases and we swallow that inside of the measure function, then we are preventing the application from handling its own exceptions. I totally agree that Glean shouldn't cause exceptions in the application, but we shouldn't interfere with an exception raised by a function defined in the application.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should definitely cancel, but I'm not sure we should default to "propagate exceptions". See my previous comments :)

The only problem is that we would be swallowing application exceptions and not Glean exceptions, and I'm not comfortable with that. If a consuming application expected a function to throw in certain cases and we swallow that inside of the measure function, then we are preventing the application from handling its own exceptions.

All right, while I don't agree with this default behaviour, your position is relatively similar to @badboy 's one.

So I'd be fine with taking the initial solution by @badboy with documentation explicitly stating that this might throw if the function throws.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize that I forgot the docs on this function, but there is a sentence on the other function. Will adopt that and then land it.

}

stop()
return returnValue
}

/**
Expand Down
18 changes: 13 additions & 5 deletions ...ore/android/src/main/java/mozilla/telemetry/glean/private/TimingDistributionMetricType.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,15 +110,23 @@ class TimingDistributionMetricType internal constructor(
}

/**
* Convenience method to simplify measuring a function or block of code
* Convenience method to simplify measuring a function or block of code.
*
* If the measured function throws, the measurement is canceled and the exception rethrown.
*/
@Suppress("TooGenericExceptionCaught")
inline fun <U> measure(funcToMeasure: () -> U): U {
val timerId = start()
try {
return funcToMeasure()
} finally {
stopAndAccumulate(timerId)

val returnValue = try {
funcToMeasure()
} catch (e: Exception) {
cancel(timerId)
throw e
}

stopAndAccumulate(timerId)
return returnValue
}

/**
Expand Down
9 changes: 3 additions & 6 deletions glean-core/android/src/test/java/mozilla/telemetry/glean/private/TimespanMetricTypeTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ class TimespanMetricTypeTest {
}

@Test
fun `measure function still measures and allows exceptions to bubble up`() {
fun `measure function bubbles up exceptions and timing is canceled`() {
// Define a timespan metric, which will be stored in "store1"
val metric = TimespanMetricType(
disabled = false,
Expand All @@ -306,8 +306,7 @@ class TimespanMetricTypeTest {
}

// Attempt to measure the function that will throw an exception. The `measure` function
// should allow the exception to bubble up while still measuring and recording to the
// metric.
// should allow the exception to bubble up, the timespan measurement is canceled.
try {
metric.measure {
testFunc()
Expand All @@ -316,9 +315,7 @@ class TimespanMetricTypeTest {
// Make sure we caught the right kind of exception: NPE
assertTrue("Exception type must match", e is NullPointerException)
} finally {
// Check that data was properly recorded, despite the exception.
assertTrue("Metric must have a value", metric.testHasValue())
assertTrue("Metric value must be greater than zero", metric.testGetValue() >= 0)
assertTrue("Metric must not have a value", !metric.testHasValue())
}
}
}
9 changes: 4 additions & 5 deletions ...android/src/test/java/mozilla/telemetry/glean/private/TimingDistributionMetricTypeTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ class TimingDistributionMetricTypeTest {
}

@Test
fun `measure function still measures and allows exceptions to bubble up`() {
fun `measure function bubbles up exceptions and timing is canceled`() {
val metric = TimingDistributionMetricType(
disabled = false,
category = "telemetry",
Expand All @@ -295,8 +295,8 @@ class TimingDistributionMetricTypeTest {
throw NullPointerException()
}

// Measure the test function, which should throw an exception. We will catch the
// exception and verify it and that we still recorded the measurement.
// Attempt to measure the function that will throw an exception. The `measure` function
// should allow the exception to bubble up, the timespan measurement is canceled.
try {
metric.measure {
testFunc()
Expand All @@ -306,8 +306,7 @@ class TimingDistributionMetricTypeTest {
assertTrue("Exception type must match", e is NullPointerException)
} finally {
// Check that data was still properly recorded even though there was an exception.
assertTrue("Metric must have a value", metric.testHasValue())
assertTrue("Metric value must be greater than zero", metric.testGetValue().sum >= 0)
assertTrue("Metric must not have a value", !metric.testHasValue())
}
}
}
21 changes: 20 additions & 1 deletion glean-core/ios/Glean/Metrics/TimespanMetric.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public class TimespanMetricType {
/// Convenience method to simplify measuring a function or block of code
///
/// - parameters:
/// * funcToMeasure: Accepts a function or closure to measure that can return a value
/// * funcToMeasure: Accepts a function or closure to measure that can return a value
public func measure<U>(funcToMeasure: () -> U) -> U {
start()
// Putting `stop` in a `defer` block guarantees it will execute at the end
Expand All @@ -101,6 +101,25 @@ public class TimespanMetricType {
return funcToMeasure()
}

/// Convenience method to simplify measuring a function or block of code
///
/// If the measured function throws, the measurement is canceled and the exception rethrown.
///
/// - parameters:
/// * funcToMeasure: Accepts a function or closure to measure that can return a value
public func measure<U>(funcToMeasure: () throws -> U) throws -> U {
start()

do {
let returnValue = try funcToMeasure()
stop()
return returnValue
} catch {
cancel()
throw error
}
}

/// Explicitly set the timespan value, in nanoseconds.
///
/// This API should only be used if your library or application requires recording
Expand Down
21 changes: 20 additions & 1 deletion glean-core/ios/Glean/Metrics/TimingDistributionMetric.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public class TimingDistributionMetricType {
/// Convenience method to simplify measuring a function or block of code
///
/// - parameters:
/// * funcToMeasure: Accepts a function or closure to measure that can return a value
/// * funcToMeasure: Accepts a function or closure to measure that can return a value
public func measure<U>(funcToMeasure: () -> U) -> U {
let timerId = start()
// Putting `stopAndAccumulate` in a `defer` block guarantees it will execute at the end
Expand All @@ -120,6 +120,25 @@ public class TimingDistributionMetricType {
return funcToMeasure()
}

/// Convenience method to simplify measuring a function or block of code
///
/// If the measured function throws, the measurement is canceled and the exception rethrown.
///
/// - parameters:
/// * funcToMeasure: Accepts a function or closure to measure that can return a value
public func measure<U>(funcToMeasure: () throws -> U) throws -> U {
let timerId = start()

do {
let returnValue = try funcToMeasure()
stopAndAccumulate(timerId)
return returnValue
} catch {
cancel(timerId)
throw error
}
}

/// Tests whether a value is stored for the metric for testing purposes only. This function will
/// attempt to await the last task (if any) writing to the the metric's storage engine before
/// returning a value.
Expand Down
32 changes: 32 additions & 0 deletions glean-core/ios/GleanTests/Metrics/TimespanMetricTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,4 +224,36 @@ class TimespanMetricTypeTests: XCTestCase {
XCTAssert(metric.testHasValue())
XCTAssert(try metric.testGetValue() >= 0)
}

func testMeasureFunctionThrows() {
let metric = TimespanMetricType(
category: "telemetry",
name: "timespan_metric",
sendInPings: ["store1"],
lifetime: .application,
disabled: false,
timeUnit: .millisecond
)

XCTAssertFalse(metric.testHasValue())

// Create a test function that throws an exception.
func testFunc() throws {
throw "invalid"
}

// Perform the measurement
do {
_ = try metric.measure {
try testFunc()
}

// The function throws, so this is unreachable
XCTAssert(false)
} catch {
// intentionally left empty
}

XCTAssertFalse(metric.testHasValue())
}
}
35 changes: 35 additions & 0 deletions glean-core/ios/GleanTests/Metrics/TimingDistributionMetricTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,4 +162,39 @@ class TimingDistributionTypeTests: XCTestCase {
let snapshot = try! metric.testGetValue()
XCTAssertEqual(3, snapshot.count)
}

func testMeasureFunctionThrows() {
let metric = TimingDistributionMetricType(
category: "telemetry",
name: "timing_distribution",
sendInPings: ["store1"],
lifetime: .application,
disabled: false,
timeUnit: .millisecond
)

XCTAssertFalse(metric.testHasValue())

// Create a test function that throws an exception.
func testFunc() throws {
throw "invalid"
}

// Measure a few times. Nothing should be recorded.
for _ in 1 ... 3 {
// Perform the measurement
do {
_ = try metric.measure {
try testFunc()
}

// The function throws, so this is unreachable
XCTAssert(false)
} catch {
// intentionally left empty
}
}

XCTAssertFalse(metric.testHasValue())
}
}