Refactor metrics package with Prometheus and InfluxDB support #428
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Split up interfaces, write vs read
The changes look large, but they're not actually that invasive. The interfaces have been split up into one write-interface and one read-interface, with
Snapshotbeing the gateway from write to read. AResettingTimermetric type which calculatesmean,50%ile,95%ile,99%ile,min, andmaxfor timers per flush interval is added - timers are reset on everySnapshot()call. This simplifies the semantics a lot.Example of splitting up an interface into one readonly 'snapshot' part, and one updatable writeonly part:
A note about concurrency
This PR makes the concurrency model clearer. We have actual meters and snapshot of meters. The
meteris the thing which can be accessed from the registry, and updates can be made to it.meters, (Gauge,Timer, etc.), it is assumed that they are accessed by different threads, making updates. Therefore, all meters update-methods (Inc,Add,Update,Clear, etc.) need to be concurrency-safe.metershave aSnapshot()method. This method is usually called from one thread, a backend-exporter. But it's fully possible to have several exporters simultaneously: therefore this method should also be concurrency-safe.TLDR:
metersare accessible via registry, all their methods must be concurrency-safe.For all
Snapshots, it is assumed that an individual exporter-thread has obtained ameterfrom the registry, and called theSnapshotmethod to obtain a readonly snapshot. This snapshot is not guaranteed to be concurrency-safe. There's no need for a snapshot to be concurrency-safe, since exporters should not share snapshots.Note, though: that by happenstance a lot of the snapshots are concurrency-safe, being unmutable minimal representations of a value. Only the more complex ones are not threadsafe, those that lazily calculate things like
Variance(),Mean().Example of how a background exporter typically works, obtaining the snapshot and sequentially accessing the non-threadsafe methods in it:
TLDR:
snapshotsare not guaranteed to be concurrency-safe (but often are).Sample changes
I also changed the
Sampletype: previously, it iterated the samples fully every timeMean(),Sum(),Min()orMax()was invoked. Since we now have readonly base data, we can just iterate it once, in the constructor, and set all four values at once.The same thing has been done for runtimehistogram.
ResettingTimer API
Back when ResettingTImer was implemented, as part of ethereum/go-ethereum#15910, Anton implemented a
Percentileson the new type. However, the method did not conform to the other existing types which also had aPercentiles.0.5to mean50%. Anton used50to mean50%.float64outputs, thus interpolating between values. A value-set of0, 10, at50%would return5, whereas Anton's would return either0or10.This PR removes the 'new' version, and uses only the 'legacy' percentiles, also for the ResettingTimer type.
The resetting timer snapshot was also defined so that it would expose the internal values. This has been removed, and getters for
Max,Min,Meanhave been added instead.Unexport types
A lot of types were exported, but do not need to be. This PR unexports quite a lot of them.