From 51524b5028a35559020856092dac2cf22959fa4c Mon Sep 17 00:00:00 2001 From: Lina Butler Date: Fri, 16 Dec 2022 11:54:31 -0800 Subject: [PATCH 1/4] docs: add ADR template This commit adds a template for our Architecture Decision Records, using the same format as Firefox Accounts. It's more structured than Nygard's status-context-decision-consequences template, but explicitly enumerates the alternatives considered and the pros and cons of each, which has been helpful in other projects (FxA, Application Services, UniFFI) that use this format. --- docs/adr/template.md | 72 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 docs/adr/template.md diff --git a/docs/adr/template.md b/docs/adr/template.md new file mode 100644 index 000000000..25696bbe7 --- /dev/null +++ b/docs/adr/template.md @@ -0,0 +1,72 @@ +# [short title of solved problem and solution] + +* Status: [proposed | rejected | accepted | deprecated | … | superseded by [ADR-0005](0005-example.md)] +* Deciders: [list everyone involved in the decision] +* Date: [YYYY-MM-DD when the decision was last updated] + +Technical Story: [description | ticket/issue URL] + +## Context and Problem Statement + +[Describe the context and problem statement, e.g., in free form using two to three sentences. You may want to articulate the problem in form of a question.] + +## Decision Drivers + +* [driver 1, e.g., a force, facing concern, …] +* [driver 2, e.g., a force, facing concern, …] +* … + +## Considered Options + +* [option 1] +* [option 2] +* [option 3] +* … + +## Decision Outcome + +Chosen option: "[option 1]", because [justification. e.g., only option, which meets k.o. criterion decision driver | which resolves force force | … | comes out best (see below)]. + +### Positive Consequences + +* [e.g., improvement of quality attribute satisfaction, follow-up decisions required, …] +* … + +### Negative Consequences + +* [e.g., compromising quality attribute, follow-up decisions required, …] +* … + +## Pros and Cons of the Options + +### [option 1] + +[example | description | pointer to more information | …] + +* Good, because [argument a] +* Good, because [argument b] +* Bad, because [argument c] +* … + +### [option 2] + +[example | description | pointer to more information | …] + +* Good, because [argument a] +* Good, because [argument b] +* Bad, because [argument c] +* … + +### [option 3] + +[example | description | pointer to more information | …] + +* Good, because [argument a] +* Good, because [argument b] +* Bad, because [argument c] +* … + +## Links + +* [Link type] [Link to ADR] +* … From 25761f9fdf6361312fc2ab68b0ee9f902a0ffcdf Mon Sep 17 00:00:00 2001 From: Lina Butler Date: Sat, 17 Dec 2022 15:55:21 -0800 Subject: [PATCH 2/4] docs: add ADR for reporting metrics in tests --- docs/adr/0000-local-and-test-metrics.md | 108 ++++++++++++++++++++++++ 1 file changed, 108 insertions(+) create mode 100644 docs/adr/0000-local-and-test-metrics.md diff --git a/docs/adr/0000-local-and-test-metrics.md b/docs/adr/0000-local-and-test-metrics.md new file mode 100644 index 000000000..3e1177326 --- /dev/null +++ b/docs/adr/0000-local-and-test-metrics.md @@ -0,0 +1,108 @@ +# Reporting metrics in local development and tests + +* Status: Proposed +* Deciders: raphael, lina +* Date: 2022-12-16 + +## Context and Problem Statement + +merino-py emits counts, histograms, and timing metrics as it runs. In production, these metrics are sent to Datadog, using an extension of the [StatsD](https://github.com/statsd/statsd) protocol called [DogStatsD](https://docs.datadoghq.com/developers/dogstatsd). For local development, merino-py provides the `metrics.dev_logger` config option, which logs outgoing metrics to the console instead of sending them to Datadog. merino-py also has integration tests that cover recording and emitting metrics. How can we avoid having the `metrics.dev_logger` setting and integration tests rely on implementation details of the `aiodogstatsd` client library to suppress reporting to Datadog? + +## Decision Drivers + +* Maintain test coverage for merino-py's metrics. +* Make it easy for engineers to see what metrics are emitted while working on the code. +* Avoid potential future compatibility issues when upgrading `aiodogstatsd`. + +## Considered Options + +* A. Do nothing. +* B. Always send metrics to a live DogStatsD agent. +* C. Change merino-py's `metrics.Client` proxy to not call into `aiodogstatsd`. +* D. Subclass `aiodogstatsd.Client` to suppress sending metrics to DogStatsD. +* E. Extend merino-py's `metrics` module with pluggable backends for production and testing. +* F. Set up a mock DogStatsD endpoint that collects and checks metrics. +* G. Ask the `aiodogstatsd` authors to expose a reporting hook as part of their public API. + +## Decision Outcome + +TODO: Fill me in once we decide! + +## Pros and Cons of the Options + +### A. Do nothing. + +This option would keep the code as it's written today: + +1. When the `metrics.dev_logger` config option is set, `metrics.configure_metrics()` sets the pseudo-private `aiodogstatsd.Client._protocol` property to an instance of `metrics._LocalDatagramLogger`. `_LocalDatagramLogger` subclasses `aiodogstatsd.client.DatagramProtocol` to log metrics instead of sending them to DogStatsD. ([DISCO-2089](https://mozilla-hub.atlassian.net/browse/DISCO-2089)) +2. merino-py's integration tests patch the pseudo-private `aiodogstatsd.Client._report()` method with a mock implementation. ([DISCO-2090](https://mozilla-hub.atlassian.net/browse/DISCO-2090)) + +* Good, because this approach currently works! +* Good, because this option doesn't require any extra work. +* Good, because `aiodogstatsd` is in maintenance mode, and unlikely to break merino-py's usage in the future. +* Bad, because relying on non-public APIs could break merino-py if `aiodogstatsd` does change its implementation in the future. +* Bad, because merino-py's metrics reporting is tightly coupled to `aiodogstatsd`. Sending metrics to a different backend would require substantial changes to the tests. + +### B. Send metrics to a live DogStatsD agent. + +This option would remove `metrics.dev_logger`. Instead, developers would need to run a DogStatsD agent locally, and configure merino-py to use that agent. + +* Good, because there would be a single path for metrics reporting in local development, testing, and production. +* Good, because this means less code in merino-py for metrics logging. +* Good, because unofficial local DogStatsD agents exist, written in [Go](https://github.com/jonmorehouse/dogstatsd-local) and [Ruby](https://github.com/drish/dogstatsd-local). +* Good, because local DogStatsD agents can log metrics in different formats. +* Bad, because developers would need to configure and run a separate service when working locally. +* Bad, because this approach requires an unofficial local DogStatsD agent. The [official DogStatsD agent](https://hub.docker.com/r/datadog/dogstatsd) is only offered as a Docker container that reports metrics to Datadog, and doesn't support custom or local-only backends. The plain StatsD agent, which supports [custom backends](https://github.com/statsd/statsd/blob/master/docs/backend.md), doesn't support the DogStatsD extensions (histograms) that merino-py uses. +* Bad, because this is only suitable for local development. Setting up a live DogStatsD agent would be more cumbersome to do in the integration tests. +* Bad, because this option assumes that merino-py will only ever report metrics to DogStatsD. + +### C. Change merino-py's `metrics.Client` proxy to not call into `aiodogstatsd` in testing. + +merino-py's `metrics.Client` class proxies calls to `aiodogstatsd.Client`. The proxy keeps track of all metrics calls for testing, and adds custom tags for feature flags. We can extend the proxy to only log metrics to the console if `metrics.dev_logger` is set, without forwarding them to the underlying `aiodogstatsd.Client`. + +* Good, because this needs minimal changes to support metrics logging for gauges, counters, histograms, and one-off timings. +* Good, because proxying calls avoids relying on implementation details of `aiodogstatsd`. +* Bad, because this approach doesn't work for `aiodogstatsd.Client.{timeit, timeit_task}()`, which return thunks that call `aiodogstatsd.Client.timing()`. Since the proxy can't intercept these calls, it would need to reimplement these methods. +* Bad, because this option assumes that merino-py will only ever report metrics to DogStatsD. + +### D. Subclass `aiodogstatsd.Client` to suppress sending metrics to StatsD in tests. + +This option would change `metrics.Client` to be a subclass of `aiodogstatsd.Client`, instead of proxying calls to it. The subclass would override the `{gauge, increment, decrement, histogram, distribution, timing}()` methods to log metrics if the f + +#### Pros + +* Good, because this would involve making the same changes as Option C needed to support methods that don't return anything. +* Good, because this works for `aiodogstatsd.Client.{timeit, timeit_task}()` out of the box, without needing to reimplement them. +* Bad, because subclassing `aiodogstatsd.Client` relies on implementation details (for example, that `timeit_task()` calling `timing()` when it's done) that could change in a future version of `aiodogstatsd`. +* Bad, because this option assumes that merino-py will only ever report metrics to DogStatsD. + +### E. Extend merino-py's `metrics` module with pluggable backends for production and testing. + +This option would turn `metrics.Client` into a generic metrics interface that could send metrics to any supported backend: DogStatsD, the console, or another metrics provider (Graphite, InfluxDB) in the future. + +* Good, because this option doesn't rely on implementation details of `aiodogstatsd`. +* Good, because this option would make it easy to add an integration test backend to verify recorded metrics. +* Good, because this option can support any number of backends, without requiring substantial changes to merino-py or the metrics interface. +* Bad, because it incurs the most engineering work out of all our options. +* Bad, because it introduces another layer of abstraction that might not be useful beyond tests or local development. +* Bad, because the new interface would need to reimplement convenience methods like `timeit_task()` from `aiodogstatsd`. + +### F. Set up a mock DogStatsD endpoint that collects and checks metrics in tests. + +This option would have the integration tests set up a UDP socket that collects, parses, and checks the recorded metrics, following [the same approach](https://github.com/Gr1N/aiodogstatsd/blob/4c363d795df04d1cc4c137307b7f91592224ed32/tests/conftest.py#L11-L13) in the `aiodogstatsd` tests. merino-py's `metrics.Client` would be configured to send metrics to this endpoint in tests. + +* Good, because integration tests wouldn't need to patch `aiodogstatsd.Client._report()`. +* Bad, because the mock socket would need to parse the DogStatsD ping, which could introduce additional bugs. +* Bad, because this replaces integration test usage only. This option doesn't address the `metrics.dev_logger` use case, and setting up a mock socket for local development would be more cumbersome. +* Bad, because this option assumes that merino-py will only ever report metrics to DogStatsD. + +### G. Ask the `aiodogstatsd` authors to expose a reporting hook as part of their public API. + +This option would involve changing `aiodogstatsd` to expose a public API for intercepting metrics before they're sent. This could involve exposing `_protocol` and `_report()`, or adding a new API surface. + +* Good, because this option wouldn't require substantial engineering work from us. +* Good, because it would provide a supported way to log and check metrics without relying on implementation details. +* Bad, because we'd need to wait for the authors to consider, approve, implement, and publish a new version with our proposal. This could take an unknown amount of time. +* Bad, because `aiodogstatsd` is stable (the last commit was December 2021), and the authors could reject making such a substantial changes to the interface. +* Bad, because this commits the `aiodogstatsd` authors to maintaining a new API, of which we would be the only consumer. +* Bad, because this option assumes that merino-py will only ever report metrics to DogStatsD. From dc6fa54695499948870c637a18e50d00e0a54c4f Mon Sep 17 00:00:00 2001 From: Lina Butler Date: Tue, 20 Dec 2022 15:28:24 -0800 Subject: [PATCH 3/4] fixup! Review feedback for ADR template. * Rank the decision drivers in order of importance. * Separate sections for the pros and cons of each option. --- docs/adr/template.md | 68 +++++++++++++++++++++++++++++--------------- 1 file changed, 45 insertions(+), 23 deletions(-) diff --git a/docs/adr/template.md b/docs/adr/template.md index 25696bbe7..268fcb669 100644 --- a/docs/adr/template.md +++ b/docs/adr/template.md @@ -12,20 +12,24 @@ Technical Story: [description | ticket/issue URL] ## Decision Drivers -* [driver 1, e.g., a force, facing concern, …] -* [driver 2, e.g., a force, facing concern, …] -* … +1. [primary driver, e.g., a force, facing concern, …] +2. [secondary driver, e.g., a force, facing concern, …] +3. … ## Considered Options -* [option 1] -* [option 2] -* [option 3] -* … +* A. [option A] +* B. [option B] +* C. [option C] +* D. … ## Decision Outcome -Chosen option: "[option 1]", because [justification. e.g., only option, which meets k.o. criterion decision driver | which resolves force force | … | comes out best (see below)]. +Chosen option: + +* A. "[option A]" + +[justification. e.g., only option, which meets primary decision driver | which resolves a force or facing concern | … | comes out best (see below)]. ### Positive Consequences @@ -39,32 +43,50 @@ Chosen option: "[option 1]", because [justification. e.g., only option, which me ## Pros and Cons of the Options -### [option 1] +### [option A] [example | description | pointer to more information | …] -* Good, because [argument a] -* Good, because [argument b] -* Bad, because [argument c] -* … +#### Pros + +* [argument for] +* [argument for] +* … -### [option 2] +#### Cons + +* [argument against] +* … + +### [option B] [example | description | pointer to more information | …] -* Good, because [argument a] -* Good, because [argument b] -* Bad, because [argument c] -* … +#### Pros + +* [argument for] +* [argument for] +* … -### [option 3] +#### Cons + +* [argument against] +* … + +### [option C] [example | description | pointer to more information | …] -* Good, because [argument a] -* Good, because [argument b] -* Bad, because [argument c] -* … +#### Pros + +* [argument for] +* [argument for] +* … + +#### Cons + +* [argument against] +* … ## Links From 166a751fe65a6e218d4763d4e7b278c9d36d6504 Mon Sep 17 00:00:00 2001 From: Lina Butler Date: Tue, 20 Dec 2022 15:33:26 -0800 Subject: [PATCH 4/4] fixup! Review feedback for metrics ADR. * Conform to ADR template changes. * Use semantic linefeeds for maintainability. * Fix truncated description for option D. --- docs/adr/0000-local-and-test-metrics.md | 174 +++++++++++++++++------- 1 file changed, 124 insertions(+), 50 deletions(-) diff --git a/docs/adr/0000-local-and-test-metrics.md b/docs/adr/0000-local-and-test-metrics.md index 3e1177326..510d23b29 100644 --- a/docs/adr/0000-local-and-test-metrics.md +++ b/docs/adr/0000-local-and-test-metrics.md @@ -2,17 +2,24 @@ * Status: Proposed * Deciders: raphael, lina -* Date: 2022-12-16 +* Date: 2022-12-20 ## Context and Problem Statement -merino-py emits counts, histograms, and timing metrics as it runs. In production, these metrics are sent to Datadog, using an extension of the [StatsD](https://github.com/statsd/statsd) protocol called [DogStatsD](https://docs.datadoghq.com/developers/dogstatsd). For local development, merino-py provides the `metrics.dev_logger` config option, which logs outgoing metrics to the console instead of sending them to Datadog. merino-py also has integration tests that cover recording and emitting metrics. How can we avoid having the `metrics.dev_logger` setting and integration tests rely on implementation details of the `aiodogstatsd` client library to suppress reporting to Datadog? +merino-py emits counts, histograms, and timing metrics as it runs. +In production, these metrics are sent to Datadog, using an extension of the +[StatsD](https://github.com/statsd/statsd) protocol called [DogStatsD](https://docs.datadoghq.com/developers/dogstatsd). +For local development, merino-py provides the `metrics.dev_logger` config option, +which logs outgoing metrics to the console instead of sending them to Datadog. +merino-py also has integration tests that cover recording and emitting metrics. +How can we avoid relying on implementation details of the `aiodogstatsd` client library +to suppress reporting to Datadog? ## Decision Drivers -* Maintain test coverage for merino-py's metrics. -* Make it easy for engineers to see what metrics are emitted while working on the code. -* Avoid potential future compatibility issues when upgrading `aiodogstatsd`. +1. Avoid future compatibility hazards when upgrading `aiodogstatsd`. +2. Make it easy for engineers to see what metrics are emitted while working on the code. +3. Maintain test coverage for merino-py's metrics. ## Considered Options @@ -34,75 +41,142 @@ TODO: Fill me in once we decide! This option would keep the code as it's written today: -1. When the `metrics.dev_logger` config option is set, `metrics.configure_metrics()` sets the pseudo-private `aiodogstatsd.Client._protocol` property to an instance of `metrics._LocalDatagramLogger`. `_LocalDatagramLogger` subclasses `aiodogstatsd.client.DatagramProtocol` to log metrics instead of sending them to DogStatsD. ([DISCO-2089](https://mozilla-hub.atlassian.net/browse/DISCO-2089)) -2. merino-py's integration tests patch the pseudo-private `aiodogstatsd.Client._report()` method with a mock implementation. ([DISCO-2090](https://mozilla-hub.atlassian.net/browse/DISCO-2090)) +1. When the `metrics.dev_logger` config option is set, `configure_metrics()` sets the pseudo-private + `aiodogstatsd.Client._protocol` property to an instance of `metrics._LocalDatagramLogger`. + `_LocalDatagramLogger` subclasses `aiodogstatsd.client.DatagramProtocol` to log metrics + instead of sending them to DogStatsD. + ([DISCO-2089](https://mozilla-hub.atlassian.net/browse/DISCO-2089)) +2. merino-py's integration tests patch the pseudo-private `aiodogstatsd.Client._report()` method + with a mock implementation. + ([DISCO-2090](https://mozilla-hub.atlassian.net/browse/DISCO-2090)) -* Good, because this approach currently works! -* Good, because this option doesn't require any extra work. -* Good, because `aiodogstatsd` is in maintenance mode, and unlikely to break merino-py's usage in the future. -* Bad, because relying on non-public APIs could break merino-py if `aiodogstatsd` does change its implementation in the future. -* Bad, because merino-py's metrics reporting is tightly coupled to `aiodogstatsd`. Sending metrics to a different backend would require substantial changes to the tests. +#### Pros + +* Currently working code! +* Doesn't require any engineering work now. +* `aiodogstatsd` is in maintenance mode, and unlikely to break merino-py's usage. + +#### Cons + +* Relying on non-public APIs could break merino-py if `aiodogstatsd` changes its implementation in the future. +* Assumes that merino-py will only ever report metrics to DogStatsD. ### B. Send metrics to a live DogStatsD agent. -This option would remove `metrics.dev_logger`. Instead, developers would need to run a DogStatsD agent locally, and configure merino-py to use that agent. +This option would remove the `metrics.dev_logger` config option. +Instead, engineers would need to run a DogStatsD agent locally, and configure merino-py to use that agent. + +#### Pros -* Good, because there would be a single path for metrics reporting in local development, testing, and production. -* Good, because this means less code in merino-py for metrics logging. -* Good, because unofficial local DogStatsD agents exist, written in [Go](https://github.com/jonmorehouse/dogstatsd-local) and [Ruby](https://github.com/drish/dogstatsd-local). -* Good, because local DogStatsD agents can log metrics in different formats. -* Bad, because developers would need to configure and run a separate service when working locally. -* Bad, because this approach requires an unofficial local DogStatsD agent. The [official DogStatsD agent](https://hub.docker.com/r/datadog/dogstatsd) is only offered as a Docker container that reports metrics to Datadog, and doesn't support custom or local-only backends. The plain StatsD agent, which supports [custom backends](https://github.com/statsd/statsd/blob/master/docs/backend.md), doesn't support the DogStatsD extensions (histograms) that merino-py uses. -* Bad, because this is only suitable for local development. Setting up a live DogStatsD agent would be more cumbersome to do in the integration tests. -* Bad, because this option assumes that merino-py will only ever report metrics to DogStatsD. +* A single path for metrics reporting in local development, testing, and production. +* Less code in merino-py for metrics logging. +* Unofficial local DogStatsD agents exist, + written in [Go](https://github.com/jonmorehouse/dogstatsd-local) and + [Ruby](https://github.com/drish/dogstatsd-local). +* Local DogStatsD agents can log metrics in different formats. + +#### Cons + +* Engineers would need to configure and run a separate service when working locally. +* Requires an unofficial local DogStatsD agent. + The [official DogStatsD agent](https://hub.docker.com/r/datadog/dogstatsd) is only offered as a Docker container + that reports metrics to Datadog, and doesn't support custom or local-only backends. + The plain StatsD agent, which + [does support custom backends](https://github.com/statsd/statsd/blob/master/docs/backend.md), + doesn't support the DogStatsD extensions (histograms) that merino-py uses. +* Only suitable for local development. + Setting up a live DogStatsD agent would be more cumbersome to do in the integration tests. +* Assumes that merino-py will only ever report metrics to DogStatsD. ### C. Change merino-py's `metrics.Client` proxy to not call into `aiodogstatsd` in testing. -merino-py's `metrics.Client` class proxies calls to `aiodogstatsd.Client`. The proxy keeps track of all metrics calls for testing, and adds custom tags for feature flags. We can extend the proxy to only log metrics to the console if `metrics.dev_logger` is set, without forwarding them to the underlying `aiodogstatsd.Client`. +merino-py's `metrics.Client` class currently proxies calls to `aiodogstatsd.Client`. +The proxy keeps track of all metrics calls for testing, and adds custom tags for feature flags. +We can extend the proxy to only log metrics to the console if `metrics.dev_logger` is set, +without forwarding them to the underlying `aiodogstatsd.Client`. -* Good, because this needs minimal changes to support metrics logging for gauges, counters, histograms, and one-off timings. -* Good, because proxying calls avoids relying on implementation details of `aiodogstatsd`. -* Bad, because this approach doesn't work for `aiodogstatsd.Client.{timeit, timeit_task}()`, which return thunks that call `aiodogstatsd.Client.timing()`. Since the proxy can't intercept these calls, it would need to reimplement these methods. -* Bad, because this option assumes that merino-py will only ever report metrics to DogStatsD. +#### Pros + +* Needs minimal changes to support metrics logging for gauges, counters, histograms, and one-off timings. +* Proxying calls avoids relying on implementation details of `aiodogstatsd`. + +#### Cons + +* Doesn't work for `aiodogstatsd.Client.{timeit, timeit_task}()`, + which return thunks that call `aiodogstatsd.Client.timing()`. + Since the proxy can't intercept these calls, it would need to reimplement these methods. +* Assumes that merino-py will only ever report metrics to DogStatsD. ### D. Subclass `aiodogstatsd.Client` to suppress sending metrics to StatsD in tests. -This option would change `metrics.Client` to be a subclass of `aiodogstatsd.Client`, instead of proxying calls to it. The subclass would override the `{gauge, increment, decrement, histogram, distribution, timing}()` methods to log metrics if the f +This option would change `metrics.Client` to be a subclass of `aiodogstatsd.Client`, instead of proxying calls to it. +The subclass would override the `{gauge, increment, decrement, histogram, distribution, timing}()` methods +to log metrics if `metrics.dev_logger` is set. #### Pros -* Good, because this would involve making the same changes as Option C needed to support methods that don't return anything. -* Good, because this works for `aiodogstatsd.Client.{timeit, timeit_task}()` out of the box, without needing to reimplement them. -* Bad, because subclassing `aiodogstatsd.Client` relies on implementation details (for example, that `timeit_task()` calling `timing()` when it's done) that could change in a future version of `aiodogstatsd`. -* Bad, because this option assumes that merino-py will only ever report metrics to DogStatsD. +* Involves making the same changes as Option C to support methods that don't return anything. +* Works for `aiodogstatsd.Client.{timeit, timeit_task}()` out of the box, without needing to reimplement them. + +#### Cons + +* Subclassing `aiodogstatsd.Client` relies on implementation details + (for example, that `timeit_task()` calls `timing()` when it's done) that could change + in a future version of `aiodogstatsd`. +* Assumes that merino-py will only ever report metrics to DogStatsD. ### E. Extend merino-py's `metrics` module with pluggable backends for production and testing. -This option would turn `metrics.Client` into a generic metrics interface that could send metrics to any supported backend: DogStatsD, the console, or another metrics provider (Graphite, InfluxDB) in the future. +This option would turn `metrics.Client` into a generic metrics interface that can send metrics to any supported +backend: DogStatsD, the console, or another metrics provider (Graphite, InfluxDB) in the future. -* Good, because this option doesn't rely on implementation details of `aiodogstatsd`. -* Good, because this option would make it easy to add an integration test backend to verify recorded metrics. -* Good, because this option can support any number of backends, without requiring substantial changes to merino-py or the metrics interface. -* Bad, because it incurs the most engineering work out of all our options. -* Bad, because it introduces another layer of abstraction that might not be useful beyond tests or local development. -* Bad, because the new interface would need to reimplement convenience methods like `timeit_task()` from `aiodogstatsd`. +#### Pros + +* Doesn't rely on implementation details of `aiodogstatsd`. +* Makes it easy to use an integration test backend to verify recorded metrics. +* Can support any number of backends, without requiring substantial changes to merino-py or the metrics interface. + +#### Cons + +* Incurs the most engineering work out of all our options. +* Introduces another layer of abstraction that might not be useful beyond tests or local development. +* The new interface would need to reimplement convenience methods like `timeit_task()` from `aiodogstatsd`. ### F. Set up a mock DogStatsD endpoint that collects and checks metrics in tests. -This option would have the integration tests set up a UDP socket that collects, parses, and checks the recorded metrics, following [the same approach](https://github.com/Gr1N/aiodogstatsd/blob/4c363d795df04d1cc4c137307b7f91592224ed32/tests/conftest.py#L11-L13) in the `aiodogstatsd` tests. merino-py's `metrics.Client` would be configured to send metrics to this endpoint in tests. +This option would have the integration tests set up a UDP socket that collects, parses, and checks +the recorded metrics, following +[the same approach](https://github.com/Gr1N/aiodogstatsd/blob/4c363d795df04d1cc4c137307b7f91592224ed32/tests/conftest.py#L11-L13) +as the `aiodogstatsd` tests. +merino-py's `metrics.Client` would be configured to send metrics to this endpoint in tests. + +#### Pros + +* Integration tests wouldn't need to patch `aiodogstatsd.Client._report()`. + +#### Cons -* Good, because integration tests wouldn't need to patch `aiodogstatsd.Client._report()`. -* Bad, because the mock socket would need to parse the DogStatsD ping, which could introduce additional bugs. -* Bad, because this replaces integration test usage only. This option doesn't address the `metrics.dev_logger` use case, and setting up a mock socket for local development would be more cumbersome. -* Bad, because this option assumes that merino-py will only ever report metrics to DogStatsD. +* The mock socket would need to parse the DogStatsD ping, which could introduce additional bugs. +* Replaces integration test usage only. + This option doesn't address the `metrics.dev_logger` use case, and setting up a mock socket for local development + would be more cumbersome. +* Assumes that merino-py will only ever report metrics to DogStatsD. ### G. Ask the `aiodogstatsd` authors to expose a reporting hook as part of their public API. -This option would involve changing `aiodogstatsd` to expose a public API for intercepting metrics before they're sent. This could involve exposing `_protocol` and `_report()`, or adding a new API surface. +This option would involve changing `aiodogstatsd` to expose a public API for intercepting metrics before they're sent. +This could involve exposing `_protocol` and `_report()`, or adding a new API surface. + +#### Pros + +* Doesn't require substantial engineering work from our team. +* Provides a supported way to log and check metrics without relying on implementation details. + +#### Cons -* Good, because this option wouldn't require substantial engineering work from us. -* Good, because it would provide a supported way to log and check metrics without relying on implementation details. -* Bad, because we'd need to wait for the authors to consider, approve, implement, and publish a new version with our proposal. This could take an unknown amount of time. -* Bad, because `aiodogstatsd` is stable (the last commit was December 2021), and the authors could reject making such a substantial changes to the interface. -* Bad, because this commits the `aiodogstatsd` authors to maintaining a new API, of which we would be the only consumer. -* Bad, because this option assumes that merino-py will only ever report metrics to DogStatsD. +* We'd need to wait for the authors to consider, approve, implement, and publish a new version with our proposal. + This could take an unknown amount of time. +* `aiodogstatsd` is stable (the last commit was December 2021), and the authors could reject + making such a substantial changes to the interface. +* Commits the `aiodogstatsd` authors to maintaining a new API, of which we would be the only consumer. +* Assumes that merino-py will only ever report metrics to DogStatsD.