Upgrade scalatest

[szymonm]

Pull Request checklist

• The commit(s) message(s) follows the contribution guidelines ?
• Tests for the changes have been added (for bug fixes / features) ?
• Docs have been added / updated (for bug fixes / features) ?

Current behavior :

Scalatest upgrade was blocking Scala 2.12 upgrade (#457 )

[szymonm]

To be continued...

[szymonm]

That wasn't fun.

[broneill]

That wasn't fun.

Why do you think we avoided doing this for so long? ;-)

[szymonm]

Why do you think we avoided doing this for so long? ;-)

Fooled myself

https://giphy.com/gifs/ihq3GYasPqshdvHcqS

[broneill]

These errors in the log are strange:
[filo-cli-akka.actor.default-dispatcher-3] ERROR 2020-07-30 07:51:57 Logger : An error occurred while trying to apply an advisor: java.lang.ClassCastException: class scala.util.Success cannot be cast to class kamon.instrumentation.context.HasContext (scala.util.Success and kamon.instrumentation.context.HasContext are in unnamed module of loader 'app')

[broneill]

These too:

[filo-cli-akka.actor.default-dispatcher-2] ERROR 2020-07-30 07:51:58  Logger : An error occurred while trying to apply an advisor: java.lang.NullPointerException
	at kamon.instrumentation.futures.scala.CallbackRunnableRunInstrumentation$.exit(FutureChainingInstrumentation.scala:83)
	at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:34)

[szymonm]

Agreed, will look into that.

[szymonm]

Did this happen before?

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.esotericsoftware.kryo.util.UnsafeUtil (file:/Users/szymon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/com/esotericsoftware/kryo/4.0.0/kryo-4.0.0.jar) to constructor java.nio.DirectByteBuffer(long,int,java.lang.Object)
WARNING: Please consider reporting this to the maintainers of com.esotericsoftware.kryo.util.UnsafeUtil
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

[broneill]

Did this happen before?

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.esotericsoftware.kryo.util.UnsafeUtil (file:/Users/szymon/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/com/esotericsoftware/kryo/4.0.0/kryo-4.0.0.jar) to constructor java.nio.DirectByteBuffer(long,int,java.lang.Object)
WARNING: Please consider reporting this to the maintainers of com.esotericsoftware.kryo.util.UnsafeUtil
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

Yes, warnings like this were already occurring.

[szymonm]

It seems to me that calling Kamon.init() in FilodbClusterNode trait may be vulnerable to different initialisation orders.
Some discussion here: kamon-io/Kamon#601

Specifically, in unit tests we should initialise Kamon with the test class and not inside specific test (see FilodbCliSpec for instance).

[velvia]

@szymonm hey thanks so much for this PR! Yes this is def why we avoided this :-p

[szymonm]

Run FiloCliSpec on develop branch and also seeing:

[2020-07-30 21:18:33,859] ERROR kamon.Init [] - Failed to attach the instrumentation agent
java.lang.reflect.InvocationTargetException: null
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at kamon.Init$class.attachInstrumentation(Init.scala:57)
	at kamon.Kamon$.attachInstrumentation(Kamon.scala:19)
	at kamon.Init$class.init(Init.scala:34)
	at kamon.Kamon$.init(Kamon.scala:19)
	at filodb.coordinator.FilodbClusterNode$class.$init$(FilodbCluster.scala:267)

It seems like people generally don't init Kamon instrumentation in the unit tests?

[broneill]

It seems like people generally don't init Kamon instrumentation in the unit tests?

Don't bother trying to fix the Kamon stuff if it already existed. It doesn't break the unit tests anyhow, but it just floods the log which is harmless. We've had issues from time to time getting Kamon initialized properly, and so this will probably need to be revisited at time point.

[broneill]

I'll go ahead and merge these changes in.

[szymonm]

The error we are seeing from akka Logger : An error occurred while trying to apply an advisor: java.lang.NullPointerException is caused by kamon, but I don't see what changed in Scalatest that revealed the issue.
My hypothesis is that some initialization of scalatests ...Spec classes changed, but I can't track it down.

Tried to move manually Kamon.init() to the start of tests initialization, but that didn't help.