As part of T228380, MediaWiki metrics need to make it into Prometheus somehow. There are a couple ways of doing this.
This task is complete when MediaWiki metrics are available in Prometheus.
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Open | None | T228380 Tech debt: sunsetting of Graphite (part 1) | |||
| Open | colewhite | T205870 Fully migrate producers off statsd | |||
| Open | colewhite | T240685 MediaWiki Prometheus support | |||
| Open | None | T249164 RFC: Better interface for generating metrics in MediaWiki | |||
| Resolved | Krinkle | T292311 Create project tag for MediaWiki-libs-Metrics | |||
| Resolved | Krinkle | T292269 Decouple Profiler class from WebRequest and RequestContext |
My two cents re: prometheus_client_php current adapters, apcu and redis, since AIUI none are optimal/desirable. There might be another way inspired by what the ruby client does (see also this PromCon 2019 talk and video). Very broadly, the idea IIRC is for each process to mmap a separate file with its own metrics, then at collection time metrics are read from the files and merged.
We recently had a conversation about this.
@Krinkle added:
a convention like metric_name.labels.go.here isn't enough as you wouldn't want prometheus to make them key_0, key_1 or something like that
@CDanis added:
we probably want the new API to encode some more semantic information somehow? probably, have some notion of what the 'base' metric name is, and what the labels are -- and some default way to sensibly flatten that into a statsd key. I think that makes more sense than starting from a statsd key and figuring out how to split out labels
One idea proposed to have some system find and expose the metrics MediaWiki would generate and dynamically create a statsd exporter config file.
Per @fgiunchedi recommendation, I put together a very basic mockup of how DirectFileStore might look in prometheus_client_php.
As I think about the cross-request persistence problem more using the client library, it's worth documenting my thoughts.
All of these have the con of having to implement the client library in the first place.
As I repeatedly reiterated, the big issue here is prometheus has a model (pull) that really doesn't work well with the PHP request management model, which is shared-nothing.
My first requirement for any observability framework does not put availability or performance of the application at risk.
One of the good things of statsd is that it's a fire-and-forget protocol where we just emit an UDP packet, hoping the receiver will get it.
A native prometheus support poses some challenges, in particular when we emit a lot of metrics and we have high concurrency - which is the case for mediawiki.
Let's get into specifics concerns I have for those 3 models:
Overall, I think the most promising route of the three is using APCu, but it comes with significant unknowns and risks of being rejected once we test it in production.
Now, AIUI switching from reporting metrics to statsd to reporting metrics to prometheus would require a lot of work in MediaWiki, and I'm worried that we could be doing a lot of work to get ourselves into a dead end.
As I have said before, I'd rather focus on what @Krinkle suggested in the patchset:
Metric reporting from MediaWiki more generally is an area I think that needs much improving and should be much more consistent and streamlined than it is today. Perhaps we could come up with a generic pattern that MediaWiki can control from its side, and we'd gradually migrate thinks towards that.
and do that in a way that allows easy, automatic translation of metrics from the statsd format to the prometheus one.
One alternative is to adopt a sidecar in the form of statsd_exporter and have it do the heavy lifting of translating MediaWiki and MW Extension metrics into Prometheus-compatible format. I see two major pain points with this solution. The first is settling on a pattern of mapping metrics to Prometheus metrics, and second is managing change over time.
To the first, there are a few options.
To to the second, the configuration has to be stored somewhere and updated when appropriate. There are a few options.
On the statsd_exporter side, a little Bash could concatenate these config files together into one config and validate the YAML before startup.
As I think about it more, it's the wire format being wholly incompatible with Prometheus format. In order to make it work, StatsD requires a lot of configuration to adequately convert, and managing that configuration will be burdensome.
If we discard StatsD as a wire format and have MediaWiki emit something more flexible, then the need for configuration can be greatly diminished.
In response to @Joe's concerns:
What happens if redis is overwhelmed/down? How can we control timeouts?
The library has a configurable timeout with a default of 0.1s. When Redis is down, ext-redis cannot establish a connection and throws an exception.
We've already had cases where a slow-responding redis (for log reception) got us in a huge outage. Having localized outages on single servers is not exactly an ideal situation either.
This is an appeal to fear. Many other things cause both local and cluster-wide outages. The risk of using any tool has to be engineered down to an acceptable level. If the risk cannot be mitigated to an appropriate level, other options must be considered.
Also: how does this library operate? can we concentrate sending data to redis in post-send, so that user-perceived latency is not impacted anyways?
This is a great question. As it stands now, no. It would have to be augmented to do this which eliminates the pro for using the client library out of the box. I would even go so far as to say the lack of this feature disqualifies out of the box use in favor of other options.
DirectFileStore: I see a handful of problems with this approach - specifically I don't think you'd be able to do what the ruby client does - you can't easily mmap a file from a request and leave it available for other processes. I think it would also be very expensive to open and close a file repeatedly (as all memory and file descriptors are request local
The ruby client was only inspiration, not a direct clone of the approach. The PHP DirectFileStore implementation opens and closes one file per request. When asked to render the metrics, the per-request files are aggregated together and added to the state file. Writing the per-request metrics file can easily be appended to the end of the request, but rendering the metrics may be a bit slow.
APCu: We make such a large use of APCu that I'm wary of interfering with the normal operations of the wikis, as we've seen scalability problems related to both APC usage and fragmentation. We'd need to see some numbers on the expected rate of writes (basically - how many writes we can expect per MediaWiki request) before we can consider it.
Myself and others share your concern with using APCu, not only for rate of use but way the library uses it doesn't seem to be a great fit. I do not think we should use it for this purpose.
DogStatsD shows some promise here. It's a statsd extension that statsd_exporter supports and enables dynamic labels. In testing, the statsd proxy doesn't support the extension, but translation is trivial if necessary.
If we proceed with DogStatsD, I could see statsd output templated as mediawiki.<extension>(.<labelValues)+.<metric> and Prometheus metrics as mediawiki_<extension>_<name> with labels and values in the appropriate places.
If we want to do it, we will still need to adapt all of the current usage of statsd in MediaWiki and decide on some standard labels to apply across the code.
I think what we need is a precise survey of what MediaWiki emits now, and from where.
Change 585032 had a related patch set uploaded (by Cwhite; owner: Cwhite):
[mediawiki/core@master] Metrics: Implement and enable statsd-exporter compatible Metrics interface
Hi @AMooney, I'd like to present this patch as the other of the two I was hoping to bring to your attention for next clinic duty... Please let me know if/how to proceed. thanks!
Change 721626 had a related patch set uploaded (by Cwhite; author: Cwhite):
[mediawiki/core@master] Metrics: Implement statsd-exporter compatible Metrics interface
Change 721627 had a related patch set uploaded (by Cwhite; author: Cwhite):
[mediawiki/core@master] Metrics: Add metrics configuration options
Change 721628 had a related patch set uploaded (by Cwhite; author: Cwhite):
[mediawiki/core@master] Metrics: Wire up MetricsFactory into ServiceWiring
Change 721629 had a related patch set uploaded (by Cwhite; author: Cwhite):
[mediawiki/core@master] Metrics: Perform MetricsFactory->flush() in emitBufferedStatsdData()
Change 721630 had a related patch set uploaded (by Cwhite; author: Cwhite):
[mediawiki/core@master] Metrics: send MetricsFactory to emit step
Change 724129 had a related patch set uploaded (by Cwhite; author: Cwhite):
[mediawiki/core@master] pass MetricsFactory instance to emitBufferedStatsdData in MWLBFactory
Change 721626 merged by jenkins-bot:
[mediawiki/core@master] Metrics: Implement statsd-exporter compatible Metrics interface
Change 585032 abandoned by Cwhite:
[mediawiki/core@master] Metrics: Implement and enable statsd-exporter compatible Metrics interface
Reason:
in favor of https://gerrit.wikimedia.org/r/c/mediawiki/core/+/721626/
A few things todo given this was merged and thus will be deployed and in a stable release:
Change 721629 abandoned by Cwhite:
[mediawiki/core@master] Metrics: Perform MetricsFactory->flush() in emitBufferedStatsdData()
Reason:
in favor of I46f0a09f4dab38fa4c9495aa2da9ecab60376ca7
Change 721628 abandoned by Cwhite:
[mediawiki/core@master] Metrics: Wire up MetricsFactory into ServiceWiring
Reason:
in favor of I46f0a09f4dab38fa4c9495aa2da9ecab60376ca7
Change 725981 had a related patch set uploaded (by Cwhite; author: Cwhite):
[mediawiki/core@master] Metrics: reduce the number of packets sent by MetricsFactory->flush()
Change 721627 merged by jenkins-bot:
[mediawiki/core@master] Metrics: Wire up MetricsFactory into ServiceWiring and emit steps
Change 725981 merged by jenkins-bot:
[mediawiki/core@master] Metrics: reduce the number of packets sent by MetricsFactory->flush()
Change 732827 had a related patch set uploaded (by Cwhite; author: Cwhite):
[operations/puppet@production] profile: add enable_relay flag to statsd exporter profile
Change 732828 had a related patch set uploaded (by Cwhite; author: Cwhite):
[operations/puppet@production] role: install statsd_exporter on canary appservers
Change 733036 had a related patch set uploaded (by Krinkle; author: Krinkle):
[mediawiki/core@master] Metrics: Add test coverage for getRenderedSamples() and send()
Change 732827 merged by Cwhite:
[operations/puppet@production] profile: add enable_relay flag to statsd exporter profile
Change 733036 merged by jenkins-bot:
[mediawiki/core@master] Metrics: Add test coverage for getRenderedSamples() and send()
Change 725372 had a related patch set uploaded (by Krinkle; author: Krinkle):
[mediawiki/core@master] Metrics: Minor doc improvements and announce feature in release notes
Change 725372 merged by jenkins-bot:
[mediawiki/core@master] Metrics: Minor doc improvements and announce feature in release notes
Just trying to figure out the status of this feature from the comments.
Would anyone in the know be able to write a summary?
Yes!
As I understand it, the next steps are:
I'm having trouble understanding what DogStatsD does and how it fits in. It is described on the linked webpage as "The easiest way to get your custom application metrics into Datadog" which is not a problem that I thought we had. Is it open source? Where is the source of it? Do they support this kind of use case? How much value does it provide compared to a DIY solution?
I reviewed the interface, and I have thoughts.
Why do names have dots on the wire, e.g. MediaWiki.SomeExtension.name_of_metric? Does DogStatsD convert the dots to underscores for Prometheus? If so, what is the point of preventing dots in MetricUtils::validateConfig()? The name policy is apparently the only thing preventing the old interface from becoming a backwards compatible wrapper around the new interface. I know it's not necessary with your migration plan, but allowing it would open up some more options for migration.
The old interface has getPerDbNameStatsdDataFactory(), and the new interface requires you to provide an associative array with "extension" provided every time a metric is updated. Both of these imply that the migrated call site is going to be verbose in the new interface and that wrappers will be required.
The term "extension" is awkward because there are callers that aren't extensions. In wfDeprecated() we use the term "component" which might be a better fit here.
I think it overuses associative arrays. Formal parameters can carry types both in static analysis and at runtime, and have better IDE integration. The flexibility advantage of associative arrays will mostly disappear in PHP 8.0 with the introduction of named parameters.
TimingMetric could be improved by having it actually measure time. Most callers don't want to be fiddling with microtime(true).
I think I would add a MediaWiki-specific stateful factory which would carry name components and labels, for the caller's convenience.
Something like
$serviceWiring = [ 'MyExtension.MetricFactory' => static function ( $services ) { // chainable + immutable, mutator-like methods clone the object return $services->getMetricFactory()->extension( 'MyExtension' ); } ]; namespace MediaWiki\Extension\MyExtension; class SpecialThing { public function __construct( $myExtensionMetricFactory ) { $this->metricFactory = $myExtensionMetricFactory ->wiki( /* default = current wiki */ ) ->name( 'Thing' ) ->label( 'color', 'green' ); } public function edit() { $timer = $this->metricFactory->startTimer( 'edit' ); ... // emit statsd metric MediaWiki.$wiki.MyExtension.Thing.green.edit // or prometheus metric MediaWiki_MyExtension_Thing_edit{wiki=$wiki, color=green} $timer->stop(); } }
Hey, thanks for following up! These are great questions.
You are correct, integration with Datadog is not the end goal. DogStatsD has the benefit of looking and behaving like StatsD on the wire, but extends the StatsD schema to include tagging which turn into Prometheus labels. This buys us the ability to deploy a statsd_exporter sidecar with no custom parsing configuration and we get Prometheus metrics out of it. We explored the possibility of configuring the statsd_exporter sidecar parsing the currently-generated metrics, but found the configuration quite lengthy and unsustainable as metrics are added/removed/changed.
Is it open source? Where is the source of it? Do they support this kind of use case?
DogStatsD is a datagram format that extends StatsD with tagging. References to code seem to be the datadog client which is Apache2 licensed.
How much value does it provide compared to a DIY solution?
This answer is fairly lengthy. In short, we already use statsd_exporter elsewhere, it understands dogstatsd, and reliably converts these datagram packets into Prometheus metrics.
I reviewed the interface, and I have thoughts.
Why do names have dots on the wire, e.g. MediaWiki.SomeExtension.name_of_metric? Does DogStatsD convert the dots to underscores for Prometheus? If so, what is the point of preventing dots in MetricUtils::validateConfig()?
Yes, the dots are converted to underscores by statsd_exporter. Dots are not supported as Prometheus metric or label names. This validation step is to maintain label integrity when StatsD is rendered:
# Prometheus
MediaWiki_SomeExtension_name_of_metric{label_1="value_1", label_2="value_2"}
# StatsD
MediaWiki.SomeExtension.name_of_metric.label_1.value_1.label_2.value_2Without maintaining name and label integrity, it is possible these metrics could be generated by the same call:
# Prometheus
MediaWiki_SomeExtension_name_of_metric{label_1="value_1", label_2="value_2"}
# StatsD
MediaWiki.SomeExtension.name.of.metric.label.1.value.1.label.2.value.2The name policy is apparently the only thing preventing the old interface from becoming a backwards compatible wrapper around the new interface. I know it's not necessary with your migration plan, but allowing it would open up some more options for migration.
The rationale for the metric library behavior is that Prometheus needs separation between metric name and labels where StatsD has no such need. The new library collects the data in a way Prometheus expects it and tries its best to digest it into a bare StatsD-compatible format. I think it's possible to provide an override setting that would allow users to set arbitrary metric namespaces to only be used by StatsD, but it feels fairly hacky.
The old interface has getPerDbNameStatsdDataFactory(), and the new interface requires you to provide an associative array with "extension" provided every time a metric is updated. Both of these imply that the migrated call site is going to be verbose in the new interface and that wrappers will be required.
Oof. Thanks for pointing this out. I was unaware of this abstraction. Seems like we would have to create a service that mirrors this pattern since it is config data being incorporated by ServiceWiring.
The term "extension" is awkward because there are callers that aren't extensions. In wfDeprecated() we use the term "component" which might be a better fit here.
This is a determination I think a core dev should make. The whole point of "extension" was to enforce uniqueness early on in the metric names to stem the possibility of naming conflicts. Metric naming conflicts are still possible in both formats due to the lack of a namespace validation feedback loop.
TimingMetric could be improved by having it actually measure time. Most callers don't want to be fiddling with microtime(true).
Good point! This seems like it would be an easy feature to add.
I think it overuses associative arrays. Formal parameters can carry types both in static analysis and at runtime, and have better IDE integration. The flexibility advantage of associative arrays will mostly disappear in PHP 8.0 with the introduction of named parameters.
I think I would add a MediaWiki-specific stateful factory which would carry name components and labels, for the caller's convenience.
Something like
$serviceWiring = [ 'MyExtension.MetricFactory' => static function ( $services ) { // chainable + immutable, mutator-like methods clone the object return $services->getMetricFactory()->extension( 'MyExtension' ); } ]; namespace MediaWiki\Extension\MyExtension; class SpecialThing { public function __construct( $myExtensionMetricFactory ) { $this->metricFactory = $myExtensionMetricFactory ->wiki( /* default = current wiki */ ) ->name( 'Thing' ) ->label( 'color', 'green' ); } public function edit() { $timer = $this->metricFactory->startTimer( 'edit' ); ... // emit statsd metric MediaWiki.$wiki.MyExtension.Thing.green.edit // or prometheus metric MediaWiki_MyExtension_Thing_edit{wiki=$wiki, color=green} $timer->stop(); } }
The overuse of associative arrays were an attempt to keep us from getting painted into a corner. I am very much interested in ways to improve the interface.
TIL extensions can register services. I like this.
From @tstarling's comment, I see a few action items:
Questions to answer:
I think more specifically services->getStatsdDataFactory(): BufferingStatsdDataFactory which is what the vast majority of MW production code uses. The per-dbname wrapper is fairly rarely used in practice (only Wikibase and Growth experiment). Supporting the per-dbname wrapper would be nice, but imho less urgent.