Clean up some warnings and a typo in a public method #137
Conversation
Codecov Report
@@ Coverage Diff @@
## master #137 +/- ##
==========================================
- Coverage 92.54% 92.52% -0.02%
==========================================
Files 51 51
Lines 872 870 -2
Branches 56 51 -5
==========================================
- Hits 807 805 -2
Misses 65 65
Continue to review full report at Codecov.
|
|
The difference between empty parameter lists between |
|
Was reading up on idiomatic Scala and it sounds like empty parameter lists should be omitted when the method has no side effects. Given this my opinion would be that we should change the trait SpanContext {
def push(span: Span): Unit
def pop(): Option[Span]
def current: Option[Span]
def clear(): Unit
}As |
|
I would prefer to get the W3C Trace Header updates out before making a breaking release. Happy to do a breaking release after that. I'm a little concerned about making a 1.0 release without going through and doing a deep clean. There's a bunch of stuff that needs to be ripped out of the akka package and I imagine a lot of the other packages are so decrepit that they should really be rejuvenated before we go to 1.0. At some point we do need to have a discussion about what we want to do with Money. I personally feel like converting Money to being a shim for OpenTelemetry (OT) and asking our users to make the switch to OT would be the most responsible solution for the library going forward. cc: @paulcleary what are your thoughts? |
| } | ||
| } | ||
|
|
||
| def propogateMDC(submittingThreadsContext: Option[Map[_, _]]): Unit = if (enabled) { | ||
| def propagateMDC(submittingThreadsContext: Option[Map[String, String]]): Unit = if (enabled) { |
There was a problem hiding this comment.
Is this the only breaking change? Can we just instead deprecate the incorrect name and add a second method with the correct name?
I am actually more concerned about the type changes being made here and what effect that will have on integrated codebases. I also don't have any context as to why we need to wildcard the type here.
There was a problem hiding this comment.
I don't know if the changes to SpanContext are source-breaking or binary-breaking. They might count also.
Old versions of slf4j returned the raw Map type with the context map.
As for the name change, that's up for debate. IMO if this is a change on the road to 1.x it's better to accept the breaking changes now, but we can also temporarily include the deprecated version until we do release that version.
There was a problem hiding this comment.
How is this breaking, just curious? The method is internal to the money runtime isn't it? I guess as long as anything that calls this method is also updated it is ok?
There was a problem hiding this comment.
It's technically a public class and used by other modules in the project to propagate MDC across thread boundaries. It's possible that an external client took a dependency on the class directly if they were tracing around some other asynchronous library not covered by one of Money's projects.
| @@ -122,5 +122,5 @@ class SpanContextWithStack() extends SpanContext { | |||
| * empties the Stack of all spans | |||
| */ | |||
|
|
|||
| override def clear: Unit = stack = List.empty[Span] | |||
| override def clear(): Unit = stack = List.empty[Span] | |||
|
I'd also not want to rev Money to 1.0 until OpenTelemetry does, so that would be another reason to avoid (or mitigate) breaking changes. |
|
Yeah that's a great point! I think everyone in this thread knows already but I created an issue related to this #138 |
Clean up most of the Scala 2.12 warnings. Many of the Scala 2.13 warnings cannot currently be addressed with code that can cross-compile with Scala 2.12.
Replace use of deprecated
Stack[Span]inSpanLocalwithList[Span]treated as a stack.Fix typo in name of
MDCSupport.propagateMDCmethod.