Skip to content

Commit

Permalink
util-stats: use service scopeSeparator in MetricBuilder
Browse files Browse the repository at this point in the history
Problem
-------
Metric Metadata uses / as the scope separator in all cases. As a result, when service owners configure a custom scope separator (ex, -) the metric names will differ in metrics.json and metric_metadata.json.

Solution
--------
Create an object in util-stats to store a scope separating character for MetricBuilder and metric_metadata.json to make use of.

Result
------
Metric names now align across metrics.json and metric_metadata.json for services with custom configured scope separators

JIRA Issues: CSL-10202

Differential Revision: https://phabricator.twitter.biz/D557994
  • Loading branch information
Matt Dannenberg authored and jenkins committed Oct 1, 2020
1 parent 26e7874 commit 7665b9e
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 1 deletion.
7 changes: 7 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ Note that ``PHAB_ID=#`` and ``RB_ID=#`` correspond to associated messages in com
Unreleased
----------

Bug Fixes
~~~~~~~~~
* util-stat: `MetricBuilder` now uses a configurable metadataScopeSeparator to align
more closely with the metrics.json api. Services with an overridden scopeSeparator will
now see that reflected in metric_metadata.json where previously it was erroneously using
/ in all cases. ``PHAB_ID=D557994``

* util-slf4j-api: Better Java interop. Deprecate `c.t.util.logging.Loggers` as Java users should be
able to use the `c.t.util.logging.Logger` companion object with less verbosity required.
``PHAB_ID=D558605``
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,21 @@ case object NoRoleSpecified extends SourceRole
case object Client extends SourceRole
case object Server extends SourceRole

/**
* finagle-stats has configurable scope separators. As this package is wrapped by finagle-stats, we
* cannot retrieve it from finagle-stats. Consequently, we created this object so that the
* scope-separator can be passed in for stringification of the MetricBuilder objects.
*/
object metadataScopeSeparator {
@volatile private var separator: String = "/"

def apply(): String = separator

private[finagle] def setSeparator(separator: String): Unit = {
this.separator = separator
}
}

/**
* A builder class used to configure settings and metadata for metrics prior to instantiating them.
* Calling any of the three build methods (counter, gauge, or histogram) will cause the metric to be
Expand Down Expand Up @@ -182,7 +197,7 @@ class MetricBuilder(
}

override def toString(): String = {
val nameString = name.mkString("/")
val nameString = name.mkString(metadataScopeSeparator())
s"MetricBuilder($keyIndicator, $description, $units, $role, $verbosity, $sourceClass, $nameString, $relativeName, $processPath, $percentiles, $statsReceiver)"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package com.twitter.finagle.stats

import org.scalatest.funsuite.AnyFunSuite

class MetricBuilderTest extends AnyFunSuite {

test("metadataScopeSeparator affects stringification") {
val mb = new MetricBuilder(name = Seq("foo", "bar"), statsReceiver = new InMemoryStatsReceiver)
assert(mb.toString.contains("foo/bar"))
metadataScopeSeparator.setSeparator("-")
assert(mb.toString.contains("foo-bar"))
metadataScopeSeparator.setSeparator("_aTerriblyLongStringForSomeReason_")
assert(mb.toString.contains("foo_aTerriblyLongStringForSomeReason_bar"))
}

}

0 comments on commit 7665b9e

Please sign in to comment.