Pre-scala 2.13: inline Box #24768

DavidLawes · 2022-03-15T17:15:14Z

What does this change?

In advance of updating the scala version we want to update as many dependencies as possible to be compatible with both 2.12 and 2.13.

We tried updating frontend scala to 2.13. One of the compilation errors was from com.gu.box. Given box is just a very thin wrapper of AtomicReference, and only used by a handful of repos, it's been suggested to inline it.

In this PR we remove Box as a dependency and define it in common. Most of the changes relate to updated imports.

One specific error caused me a bit of confusion:

Inlining Box resulted in a failing test in DurationMetricTest --> the test failed because the list length was not expected (ref: https://teamcity.gutools.co.uk/viewLog.html?buildId=752344&buildTypeId=dotcom_master&tab=buildLog&branch_dotcom=dlawes%2Fupdate-guBox&_focus=15032)
I could see that FrontendMetrics was still attempting to add new data points to the head of the list, but the existing data points could not be resolved (unbound placeholder parameter error).
I noticed an equivalent function in the same file: https://github.com/guardian/frontend/blob/dlawes/update-guBox/common/app/metrics/FrontendMetrics.scala#L162. This was not covered by tests; I added tests and saw it was also failing.

^^ to resolve the failing tests we had to update the methods defined in the Box class. There was actually an error in the library itself (around variable names and scopes), which meant that we were only ever altering or modifying the original value for our ref, never the latest. Now that these methods in the local Box class are updated (or removed, if not being used), our tests pass.

Does this change need to be reproduced in dotcom-rendering ?

No
Yes (please indicate your plans for DCR Implementation)

Screenshots

N/A

What is the value of this and can you measure success?

Checklist

Does this affect other platforms?

AMP
Apps
Other (please specify)

Does this affect GLabs Paid Content Pages? Should it have support for Paid Content?

No
Yes (please give details)

Does this change break ad-free?

No
It did, but tests caught it and I fixed it
It did, but there was no test coverage so I added that then fixed it

Does this change update the version of CAPI we're using?

No, all the existing database files are just fine
Yes, and I have re-run all the tests locally and checked in all the updated data/database/xyz files

Accessibility test checklist

Tested

Locally
On CODE (optional)

DavidLawes · 2022-03-16T12:18:15Z

common/app/common/Box.scala

@@ -0,0 +1,46 @@
+package common


This is copypasta from https://github.com/guardian/box/blob/master/src/main/scala/com/gu/Box.scala

DavidLawes · 2022-03-16T12:18:59Z

common/test/common/BoxTest.scala

@@ -0,0 +1,16 @@
+package common


This is copypasta from https://github.com/guardian/box/blob/master/src/test/scala/com/gu/BoxSpec.scala

DavidLawes · 2022-03-16T12:21:05Z

common/test/metrics/DurationMetricTest.scala

@@ -44,4 +45,44 @@ class DurationMetricTest extends FlatSpec with Matchers {
    dataPoints.length should be(7)
    dataPoints.splitAt(4)._1 should be(allMetrics)
  }
+
+  "SamplerMetric" should "start off empty" in {


The SamplerMetric functionality is almost identical to DurationMetric count, but the recording of the metric/sample is handled by 2 different functions. These tests cover the SamplerMetric recording functionality, they were missing before and helped me try to debug what was going on

DavidLawes · 2022-03-16T12:23:09Z

common/app/metrics/FrontendMetrics.scala

@@ -128,7 +127,7 @@ final case class DurationMetric(override val name: String, override val metricUn
  }

  // Public for tests.
-  def record(dataPoint: DurationDataPoint): Unit = dataPoints.alter(dataPoint :: _)
+  def record(dataPoint: DurationDataPoint): Unit = dataPoints.alter(dataPoint :: dataPoints.get())


Without this change I was seeing an 'unbound placeholder parameter error' which I think means scala didn't know how to interpret _. What I think we want from this function is to append new data points to the list (without this change we just overwrote any existing datapoints, List always had length of 1).

I don't understand why my change introduced this error though. Any ideas?

I'm struggling to understand how it would have understood _ in the first place to be honest!

DavidLawes · 2022-03-16T12:24:31Z

common/app/metrics/FrontendMetrics.scala

@@ -160,7 +159,7 @@ final case class SamplerMetric(override val name: String, override val metricUni
  }

  def recordSample(sampleValue: Double, sampleTime: DateTime): Future[List[SampledDataPoint]] =
-    dataPoints.alter(SampledDataPoint(sampleValue, sampleTime) :: _)
+    dataPoints.alter(SampledDataPoint(sampleValue, sampleTime) :: dataPoints.get())


I was curious if this function exhibited the same behaviour as record above (i.e. _ results in an 'unbound placeholder parameter error'). After writing tests I confirmed this and believe we now add new sampled data points to the head of the list.

Again, not sure why my change resulted in the need for this change (although here it could've existed already, there was no test coverage)

The box class used the same reference for the initial value and inside the alter method. The result was an error in the alter method that prevent us from ever updating correctly. This change amends the box class and reverts FrontendMetrics. Co-authored-by: David Furey <david.furey@guardian.co.uk>

DavidLawes · 2022-03-21T10:01:09Z

common/app/common/Box.scala

+  def apply[T](initialValue: T): Box[T] = new AtomicRefBox[T](initialValue)
+}
+
+private class AtomicRefBox[T](initialValue: T) extends Box[T] {


Note the omission of modify, map and flatMap (https://github.com/guardian/box/blob/master/src/main/scala/com/gu/Box.scala#L38). These functions aren't used in frontend and the modify function has not been implemented correctly in box.

DavidLawes · 2022-03-21T10:03:27Z

common/app/common/Box.scala

+}
+
+private class AtomicRefBox[T](initialValue: T) extends Box[T] {
+  private val ref: AtomicReference[T] = new AtomicReference[T](initialValue)


Note the renaming of the initial value. In box this is named t meaning that in the alter and modify methods t was always resolving to this initial value.

This comment and the above one are good comments, would the be useful to future devs coming to this code?

Good shout, will add :)

jamesgorrie · 2022-03-22T10:27:30Z

common/app/common/Box.scala

+}
+
+private class AtomicRefBox[T](initialValue: T) extends Box[T] {
+  private val ref: AtomicReference[T] = new AtomicReference[T](initialValue)


This comment and the above one are good comments, would the be useful to future devs coming to this code?

prout-bot · 2022-03-22T21:36:25Z

Seen on PROD (merged by @DavidLawes 7 hours, 29 minutes and 40 seconds ago)

Check your changes on www.theguardian.com ✔️
Keep an eye on the deploy dashboard 📉

inline Box class

fcaefba

DavidLawes added the Scala 2.13 See https://github.com/guardian/maintaining-scala-projects/issues/2 label Mar 15, 2022

DavidLawes linked an issue Mar 15, 2022 that may be closed by this pull request

box #24764

Closed

DavidLawes added 2 commits March 15, 2022 18:14

fix unbound placeholder parameter error

cb15df3

fix unbound placeholder parameter error for SamplerMetric

0a1d762

DavidLawes commented Mar 16, 2022

View reviewed changes

DavidLawes marked this pull request as ready for review March 16, 2022 12:37

DavidLawes requested review from a team as code owners March 16, 2022 12:37

DavidLawes commented Mar 21, 2022

View reviewed changes

jamesgorrie approved these changes Mar 22, 2022

View reviewed changes

add comments

c4fed10

DavidLawes merged commit 995d8b9 into main Mar 22, 2022

DavidLawes deleted the dlawes/update-guBox branch March 22, 2022 14:06

prout-bot added Seen-on-PROD and removed Scala 2.13 See https://github.com/guardian/maintaining-scala-projects/issues/2 labels Mar 22, 2022

DavidLawes mentioned this pull request Apr 1, 2022

Inline Box library guardian/grid#3698

Closed

4 tasks

ioannakok mentioned this pull request Aug 23, 2022

Switch to Scala 2.13 #25190

Merged

2 tasks

rtyley added the Scala 2.13 See https://github.com/guardian/maintaining-scala-projects/issues/2 label Feb 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pre-scala 2.13: inline Box #24768

Pre-scala 2.13: inline Box #24768

DavidLawes commented Mar 15, 2022 •

edited

Loading

DavidLawes Mar 16, 2022 •

edited

Loading

DavidLawes Mar 16, 2022

DavidLawes Mar 16, 2022

DavidLawes Mar 16, 2022

OllysCoding Mar 17, 2022

DavidLawes Mar 16, 2022 •

edited

Loading

DavidLawes Mar 21, 2022

DavidLawes Mar 21, 2022

jamesgorrie Mar 22, 2022

DavidLawes Mar 22, 2022

jamesgorrie Mar 22, 2022

prout-bot commented Mar 22, 2022

Pre-scala 2.13: inline Box #24768

Pre-scala 2.13: inline Box #24768

Conversation

DavidLawes commented Mar 15, 2022 • edited Loading

What does this change?

Does this change need to be reproduced in dotcom-rendering ?

Screenshots

What is the value of this and can you measure success?

Checklist

Does this affect other platforms?

Does this affect GLabs Paid Content Pages? Should it have support for Paid Content?

Does this change break ad-free?

Does this change update the version of CAPI we're using?

Accessibility test checklist

Tested

DavidLawes Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidLawes Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prout-bot commented Mar 22, 2022

DavidLawes commented Mar 15, 2022 •

edited

Loading

DavidLawes Mar 16, 2022 •

edited

Loading

DavidLawes Mar 16, 2022 •

edited

Loading