Skip to content
This repository has been archived by the owner on Apr 14, 2023. It is now read-only.

Refactor the metric classes #15

Merged
merged 3 commits into from
Aug 10, 2012
Merged

Refactor the metric classes #15

merged 3 commits into from
Aug 10, 2012

Conversation

daithiocrualaoich
Copy link

  • Genericize GaugeMetric
  • Factor common implementation to AbstractMetric
  • Add increment method to CountMetric
  • Correct some sloppy attribute naming
  • Add some tests

* Genericize GaugeMetric
* Factor common implementation to AbstractMetric
* Add increment method to CountMetric
* Correct some sloppy attribute naming
* Add some tests
@daithiocrualaoich
Copy link
Author

@philwills Any chance you can once over this for me?


def count: Long = getCount()
def asJson: StatusMetric = StatusMetric(group, master map { _.definition }, name, `type`, title, description)

Choose a reason for hiding this comment

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

Don't we need the value in the JSON (I'm probably missing something here due to just looking at diffs)?

Copy link
Author

Choose a reason for hiding this comment

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

Hard to see in the diff.

count was a convenience method only. The getCount came out in the JSON in the value attribute anyway. Count is also wrong language for a gauge. So it has all disappeared in favour of a getValue: () => T that gets passed into the constructor.

The sourcefile in full: https://github.com/guardian/guardian-management/blob/174f4332766cde372d361a80891ab7a359f86050/management/src/main/scala/com/gu/management/metrics.scala

The previous sourcefile: https://github.com/guardian/guardian-management/blob/0517d9bdf9b1799a0fb0dec370bc3eff6e7c8947/management/src/main/scala/com/gu/management/metrics.scala

@philwills
Copy link

👍

daithiocrualaoich added a commit that referenced this pull request Aug 10, 2012
Refactor the metric classes
@daithiocrualaoich daithiocrualaoich merged commit 288ff28 into master Aug 10, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants