jnioche opened a new pull request, #2229:
URL: https://github.com/apache/stormcrawler/pull/2229
`AbstractStatusUpdaterBolt` registered `cache.hits` and `cache.misses` as
gauges whose supplier returned the count **and reset it to 0** on every read.
That works with the V1 metrics API, where a single consumer reads each time
bucket. It is wrong under V2 (`stormcrawler.metrics.version: "v2"` or
`"both"`): Dropwizard gauges must be side-effect free, and every V2 reporter
(Console, CSV, JMX, Graphite, a Prometheus endpoint scraped by several servers,
or someone curling `/metrics` by hand) took the counts away from the others.
### Changes
- The two gauges are replaced by a scoped counter `cache`, with scopes
`hits` and `misses`, registered through `CrawlerMetrics.registerCounter`. The
handles are created once in `prepare()` and incremented where the `long` fields
used to be.
- `cache.size` stays a gauge.
- The counts were plain `long` fields read and reset from the reporter
thread. They are now held by `CountMetric` (V1) and thread-safe Codahale
`Counter`s (V2).
### Metric names and semantics
| | Before | After |
|---|---|---|
| **V1** | two gauges, `cache.hits` / `cache.misses`, per time bucket | one
`MultiCountMetric` named `cache` (`{hits, misses}`), per time bucket |
| **V2** | gauges `cache.hits` / `cache.misses`, reset on read | counters
`cache.hits` / `cache.misses`, **cumulative** |
- **V1:** the OpenSearch, opensearch-java, Solr and SQL `MetricsConsumer`s
flatten map values as `name.key`, so they keep storing `cache.hits` and
`cache.misses` with the same per-bucket values. Both scopes are created in
`prepare()`, so they are reported as 0 when idle, like the gauges were. A
custom V1 consumer that only handles numbers would now receive one `cache` map
instead of two numbers.
- **V2:** the names are unchanged, but the values are now cumulative.
**Dashboards that used `cache.hits` / `cache.misses` as per-interval values
under V2 should switch to `rate()` / `increase()`.**
### Tests
Two tests are added to `AbstractStatusUpdaterBoltTest`:
- `cacheCountsAreCumulativeUnderV2`: uses a real `MetricRegistry`. It checks
that `cache.size` is the only gauge, that repeated reads see the same counts,
and that the counts keep growing.
- `cacheCountsArePerTimeBucketUnderV1`: catches the `cache` metric and
checks that it reports `{hits=2, misses=1}` and then `{hits=0, misses=0}` after
`getValueAndReset()`.
Both tests fail against the previous implementation.
### For all changes
- [ ] Is there a issue associated with this PR? Is it referenced in the
commit message?
- [ ] Does your PR title start with `#XXXX` where `XXXX` is the issue number
you are trying to resolve?
- [x] Has your PR been rebased against the latest commit within the target
branch (typically main)?
- [x] Is your initial contribution a single, squashed commit?
- [x] Is the code properly formatted with `mvn git-code-format:format-code
-Dgcf.globPattern="**/*" -Dskip.format.code=false`?
### For code changes
- [ ] Have you ensured that the full suite of tests is executed via `mvn
clean verify`? (only `AbstractStatusUpdaterBoltTest` and `CrawlerMetricsTest`
were run locally, plus checkstyle and the format check)
- [x] Have you written or updated unit tests to verify your changes?
- [x] If adding new dependencies to the code, are these dependencies
licensed in a way that is compatible for inclusion under [ASF
2.0](http://www.apache.org/legal/resolved.html#category-a)? (no new
dependencies)
- [x] If applicable, have you updated the LICENSE file, including the main
LICENSE file? (not applicable)
- [x] If applicable, have you updated the NOTICE file, including the main
NOTICE file? (not applicable)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
