158-simplify-cloudwatch-to-graphite-app-remove-leadbutt-dependency #14

benvand · 2018-09-20T08:37:10Z

Refactor/ simplification of the app:

Remove pipe-watchdog.py entirely
- We're no longer using pipes to direct the output. An error should be output to the cloudwatch error logs on failure cf logs --recent digitalmarketplace-cloudwatch-to-graphite-preview
- Remove this from the run command in manifest.yml.example
Add flake8 dependency and .flake8 file in line with GDS recommendations
Activate travis service on repo to check Flake8 rules and requirements files
- Add .travis.yml
Add Makefile with shortcuts for running requirements freeze and flake8
Update README.md
Change requirements handling
- Add requirements-app.txt
- Ensure requirements.txt contains full pinned requirements
- Add flake8 requirement for devs
Rewrite app.py
- Add function descriptions to all functions
- Remove dependency on leadbutt, signal and subprocess
- Use boto3 instead of leadbutts boto (https://github.com/crccheck/cloudwatch-to-graphite/blob/master/leadbutt.py#L25)
- Remove get_timestamp method. The same can be achieved with time.time
- logger.warn is deprecated, use logger.warning instead
- Bring string formatting in line with rest of strings in file
- Rename initialize_metrics to create_hostedgraphite_base_metrics for better description
- Remove call_leadbutt function and replace with code calling aws ourselves. Extrapolated from (https://github.com/crccheck/cloudwatch-to-graphite/blob/master/leadbutt.py)
- Change loglevel to INFO this is all that is required now we're not piping output
- Move retry decorator on to wrapper method in if __name__ block
Update config.yaml.j2 template
- Dimensions default = {} to avoid type manipulations
- Remove Auth section from config. This can be hard coded until we have any need to specify it as a config variable
- Remove Options.Count again, this can be hard coded. It's only used in one place and we've never needed to change it
Replace retrying with tenacity
- retrying is stale. tenacity is a community maintained fork. ([Reminder] The project is nearly obsolete rholder/retrying#79)

## TL:DR;

Extrapolate code out of leadbutt
This code drops leadbutts retry functionality. Boto3 has the capacity to retry failed attempts. We should prefer this.

katstevens

Nice work on this - just a few minor tweaks suggested.

katstevens · 2018-09-24T10:09:46Z

README.md

@@ -3,7 +3,8 @@ Digital Marketplace Cloudwatch to Graphite

 ## Purpose

-Ships Cloudwatch metrics to Hosted Graphite using https://github.com/crccheck/cloudwatch-to-graphite.
+Ships Cloudwatch metrics to Hosted Graphite.
+Heavily draws on the work of https://github.com/crccheck/cloudwatch-to-graphite


Can we be more specific and say which bits we have been 'inspired by'?

katstevens · 2018-09-24T10:16:19Z

app.py

-@retry(wait_fixed=60000, retry_on_result=lambda res: res is None)
-def call_leadbutt():
+def create_hostedgraphite_base_metrics(config):
+    """Take a metric entry from the config format it into a base metric used to initialise the metric on hostedgraphite.


Could we simplify this to """Initialise each base metric in the config with a timestamp"""?

Almost, but we're also initialising it with a base number.

They come out like:
cloudwatch.request_time_buckets.preview.antivirus-api.request_time_bucket_0.sum 0.0 1537113600
{metric_name} {base_value} {timestamp}

Setting a starting point for the metric.

OK - Initialise each base metric in the config with a timestamp and base number then? 😸 The current docstring is a bit hard to read.

katstevens · 2018-09-24T10:58:32Z

app.py

+
+
+def get_metric_from_cloudwatch(client, metric):
+    """Call the client once for th supplied metric."""


katstevens · 2018-09-24T11:01:48Z

tests/test_app.py

+    def setup_method(self):
+        # Mock out boto3 cloudwatch client
+        self.client = boto3.client('cloudwatch', region_name="eu-west-1")
+        stubber = Stubber(self.client)


Interesting! Is this a better option than moto?

I think so in so much as it prevents another dependency and should be supported for the lifetime of the boto3 lib. Pretty nifty, but it hasn't been around for that long. I expect the dependency in utils was added before this existed.

katstevens · 2018-09-24T11:03:51Z

tests/test_app.py

+
+        # Mock out send_to_hostedgraphite method
+        self.send_to_hostedgraphite_mock = mock.patch('app.send_to_hostedgraphite')
+        self.send_to_hostedgraphite_mock.start()


Should we have an equivalent .stop() in a teardown_method()?

Good shout 👍 Done

katstevens · 2018-09-24T12:40:38Z

README.md

@@ -3,9 +3,10 @@ Digital Marketplace Cloudwatch to Graphite

 ## Purpose

-Ships Cloudwatch metrics to Hosted Graphite using https://github.com/crccheck/cloudwatch-to-graphite.
+Ships Cloudwatch metrics to Hosted Graphite.
+Heavily on the work of https://github.com/crccheck/cloudwatch-to-graphite and the `leadbutt` script. Particularly config parsing to define metrics.


👍 (Though I think it's now missing a word: Heavily based?)

katstevens

👍

Refactor/ simplification of the app: * Remove `pipe-watchdog.py` entirely * We're no longer using pipes to direct the output. An error should be output to the cloudwatch error logs on failure `cf logs --recent digitalmarketplace-cloudwatch-to-graphite-preview` * Remove this from the run command in `manifest.yml.example` * Add `flake8` dependency and `.flake8` file in line with GDS recommendations * Activate travis service on repo to check Flake8 rules and requirements files * Add `.travis.yml` * Add `Makefile` with shortcuts for running requirements freeze and flake8 * Update `README.md` * Change requirements handling * Add `requirements-app.txt` * Ensure `requirements.txt` contains full pinned requirements * Add `flake8` requirement for devs * Rewrite `app.py` * Add function descriptions to all functions * Remove dependency on `leadbutt`, `signal` and `subprocess` * Use `boto3` instead of `leadbutt`s `boto` (https://github.com/crccheck/cloudwatch-to-graphite/blob/master/leadbutt.py#L25) * Remove `get_timestamp` method. The same can be achieved with `time.time` * `logger.warn` is deprecated, use `logger.warning` instead * Bring string formatting in line with rest of strings in file * Rename `initialize_metrics ` to `create_hostedgraphite_base_metrics` for better description * Remove `call_leadbutt` function and replace with code calling aws ourselves. Extrapolated from (https://github.com/crccheck/cloudwatch-to-graphite/blob/master/leadbutt.py) * Change `loglevel` to `INFO` this is all that is required now we're not piping output * Move retry decorator on to wrapper method in `if __name__` block * Update `config.yaml.j2` template * `Dimensions` default = `{}` to avoid type manipulations * Remove `Auth` section from config. This can be hard coded until we have any need to specify it as a config variable * Remove `Options.Count` again, this can be hard coded. It's only used in one place and we've never needed to change it Extrapolate code out of leadbutt This code drops leadbutts retry functionality. Boto3 has the capacity to retry failed attempts. We should prefer this.

As per rholder/retrying#79 retrying is dead. Tenaciy is a community maintained fork. This also fixes a problem with retry swallowing exceptions.

benvand force-pushed the 158-simplify-cloudwatch-to-graphite-app-remove-leadbutt-dependency branch 8 times, most recently from 9fcec3a to e03ef5f Compare September 21, 2018 14:12

benvand changed the title ~~158-simplify-cloudwatch-to-graphite-app-remove-leadbutt-dependency~~ [Tests are a WIP] 158-simplify-cloudwatch-to-graphite-app-remove-leadbutt-dependency Sep 21, 2018

lfdebrux self-requested a review September 21, 2018 14:51

benvand force-pushed the 158-simplify-cloudwatch-to-graphite-app-remove-leadbutt-dependency branch 6 times, most recently from 5e340fd to 42d6911 Compare September 23, 2018 08:25

benvand changed the title ~~[Tests are a WIP] 158-simplify-cloudwatch-to-graphite-app-remove-leadbutt-dependency~~ 158-simplify-cloudwatch-to-graphite-app-remove-leadbutt-dependency Sep 23, 2018

katstevens reviewed Sep 24, 2018

View reviewed changes

benvand force-pushed the 158-simplify-cloudwatch-to-graphite-app-remove-leadbutt-dependency branch 3 times, most recently from c6f2d27 to dd64f14 Compare September 24, 2018 11:41

katstevens reviewed Sep 24, 2018

View reviewed changes

katstevens approved these changes Sep 25, 2018

View reviewed changes

benvand force-pushed the 158-simplify-cloudwatch-to-graphite-app-remove-leadbutt-dependency branch from dd64f14 to 2c075f3 Compare September 25, 2018 11:56

benvand added 3 commits September 25, 2018 18:19

Replace retrying with tenacity

f2deab6

As per rholder/retrying#79 retrying is dead. Tenaciy is a community maintained fork. This also fixes a problem with retry swallowing exceptions.

Tests

20acba6

benvand force-pushed the 158-simplify-cloudwatch-to-graphite-app-remove-leadbutt-dependency branch from 2c075f3 to 20acba6 Compare September 25, 2018 17:19

benvand merged commit 08a9730 into master Sep 25, 2018

benvand deleted the 158-simplify-cloudwatch-to-graphite-app-remove-leadbutt-dependency branch September 25, 2018 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

158-simplify-cloudwatch-to-graphite-app-remove-leadbutt-dependency #14

158-simplify-cloudwatch-to-graphite-app-remove-leadbutt-dependency #14

benvand commented Sep 20, 2018 •

edited

Loading

katstevens left a comment

katstevens Sep 24, 2018

benvand Sep 24, 2018

katstevens Sep 24, 2018

benvand Sep 24, 2018

katstevens Sep 24, 2018 •

edited

Loading

katstevens Sep 24, 2018

katstevens Sep 24, 2018

benvand Sep 24, 2018

katstevens Sep 24, 2018

benvand Sep 24, 2018

katstevens Sep 24, 2018

katstevens left a comment



		def get_metric_from_cloudwatch(client, metric):
		"""Call the client once for th supplied metric."""

158-simplify-cloudwatch-to-graphite-app-remove-leadbutt-dependency #14

158-simplify-cloudwatch-to-graphite-app-remove-leadbutt-dependency #14

Conversation

benvand commented Sep 20, 2018 • edited Loading

katstevens left a 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

katstevens Sep 24, 2018 • 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

Choose a reason for hiding this comment

katstevens left a comment

Choose a reason for hiding this comment

benvand commented Sep 20, 2018 •

edited

Loading

katstevens Sep 24, 2018 •

edited

Loading