-
Notifications
You must be signed in to change notification settings - Fork 830
improve code quality, part 1 #1808
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
bf7d0dd
add claude
zeitlinger e1fa169
add CODE_QUALITY_IMPROVEMENTS.md
zeitlinger e5fc764
add tests
zeitlinger b517091
refactor: simplify DropwizardExports by extending AbstractDropwizardE…
zeitlinger 164c732
fix
zeitlinger 6f5bba1
fix
zeitlinger 94fd6d2
fix
zeitlinger 98c11fb
how to use super linter
zeitlinger 0f4b0b1
fix
zeitlinger d1855b3
fix
zeitlinger File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| # CLAUDE.md | ||
|
|
||
| This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. | ||
|
|
||
| ## Build Commands | ||
|
|
||
| This project uses Maven with mise for task automation. The Maven wrapper (`./mvnw`) is used for all builds. | ||
|
|
||
| ```bash | ||
| # Full CI build (clean + install + all checks) | ||
| mise run ci | ||
|
|
||
| # Quick compile without tests or checks (fastest for development) | ||
| mise run compile | ||
|
|
||
| # Run unit tests only (skips formatting/coverage/checkstyle checks) | ||
| mise run test | ||
|
|
||
| # Run all tests including integration tests | ||
| mise run test-all | ||
|
|
||
| # Format code with Google Java Format | ||
| mise run format | ||
| # or directly: ./mvnw spotless:apply | ||
|
|
||
| # Run a single test class | ||
| ./mvnw test -Dtest=CounterTest -Dspotless.check.skip=true -Dcoverage.skip=true -Dcheckstyle.skip=true | ||
|
|
||
| # Run a single test method | ||
| ./mvnw test -Dtest=CounterTest#testIncrement -Dspotless.check.skip=true -Dcoverage.skip=true -Dcheckstyle.skip=true | ||
|
|
||
| # Run tests in a specific module | ||
| ./mvnw test -pl prometheus-metrics-core -Dspotless.check.skip=true -Dcoverage.skip=true -Dcheckstyle.skip=true | ||
|
|
||
| # Regenerate protobuf classes (after protobuf dependency update) | ||
| mise run generate | ||
| ``` | ||
|
|
||
| ## Architecture | ||
|
|
||
| The library follows a layered architecture where metrics flow from core types through a registry to exporters: | ||
|
|
||
| ``` | ||
| prometheus-metrics-core (user-facing API) | ||
| │ | ||
| ▼ collect() | ||
| prometheus-metrics-model (immutable snapshots) | ||
| │ | ||
| ▼ | ||
| prometheus-metrics-exposition-formats (text/protobuf/OpenMetrics) | ||
| │ | ||
| ▼ | ||
| Exporters (httpserver, servlet, pushgateway, opentelemetry) | ||
| ``` | ||
|
|
||
| ### Key Modules | ||
|
|
||
| - **prometheus-metrics-core**: User-facing metric types (Counter, Gauge, Histogram, Summary, Info, StateSet). All metrics implement the `Collector` interface with a `collect()` method. | ||
| - **prometheus-metrics-model**: Internal read-only immutable snapshot types returned by `collect()`. Contains `PrometheusRegistry` for metric registration. | ||
| - **prometheus-metrics-config**: Runtime configuration via properties files or system properties. | ||
| - **prometheus-metrics-exposition-formats**: Converts snapshots to Prometheus exposition formats. | ||
| - **prometheus-metrics-tracer**: Exemplar support with OpenTelemetry tracing integration. | ||
| - **prometheus-metrics-simpleclient-bridge**: Allows legacy simpleclient 0.16.0 metrics to work with the new registry. | ||
|
|
||
| ### Instrumentation Modules | ||
|
|
||
| Pre-built instrumentations: `prometheus-metrics-instrumentation-jvm`, `-caffeine`, `-guava`, `-dropwizard`, `-dropwizard5`. | ||
|
|
||
| ## Code Style | ||
|
|
||
| - **Formatter**: Google Java Format (enforced via Spotless) | ||
| - **Line length**: 100 characters | ||
| - **Indentation**: 2 spaces | ||
| - **Static analysis**: Error Prone with NullAway (`io.prometheus.metrics` package) | ||
| - **Logger naming**: Logger fields must be named `logger` (not `log`, `LOG`, or `LOGGER`) | ||
| - **Assertions in tests**: Use static imports from AssertJ (`import static org.assertj.core.api.Assertions.assertThat`) | ||
| - **Empty catch blocks**: Use `ignored` as the exception variable name | ||
|
|
||
| ## Linting and Validation | ||
|
|
||
| - **IMPORTANT**: Always run `mise run build` after modifying Java files to ensure all lints, code formatting (Spotless), static analysis (Error Prone), and checkstyle checks pass | ||
| - **IMPORTANT**: Always run `mise run lint:super-linter` after modifying non-Java files (YAML, Markdown, shell scripts, JSON, etc.) | ||
| - Super-linter is configured to only show ERROR-level messages via `LOG_LEVEL=ERROR` in `.github/super-linter.env` | ||
| - Local super-linter version is pinned to match CI (see `.mise/tasks/lint/super-linter.sh`) | ||
|
|
||
| ## Testing | ||
|
|
||
| - JUnit 5 (Jupiter) with `@Test` annotations | ||
| - AssertJ for fluent assertions | ||
| - Mockito for mocking | ||
| - Integration tests are in `integration-tests/` and run during `verify` phase | ||
| - Acceptance tests use OATs framework: `mise run acceptance-test` | ||
|
|
||
| ## Java Version | ||
|
|
||
| Source compatibility: Java 8. Tests run on Java 25 (configured in `mise.toml`). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| # Code Quality Improvement Plan | ||
|
|
||
| This document tracks code quality improvements for the Prometheus Java Client library. Work through these items incrementally across sessions. | ||
|
|
||
| ## High Priority | ||
|
|
||
| ### 1. Add Missing Test Coverage for Exporter Modules | ||
| - [x] `prometheus-metrics-exporter-common` - base module, no tests | ||
| - [x] `prometheus-metrics-exporter-servlet-jakarta` - no tests | ||
| - [x] `prometheus-metrics-exporter-servlet-javax` - no tests | ||
| - [x] `prometheus-metrics-exporter-opentelemetry-otel-agent-resources` - no tests | ||
|
|
||
| ### 2. Eliminate Dropwizard Module Duplication | ||
| - [x] Create shared base class or use generics for `prometheus-metrics-instrumentation-dropwizard` and `prometheus-metrics-instrumentation-dropwizard5` (~297 lines each, nearly identical) | ||
|
|
||
| ### 3. Address Technical Debt (TODOs) | ||
| - [ ] `prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/Histogram.java:965` - "reset interval isn't tested yet" | ||
| - [ ] `prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/Summary.java:205` - "Exemplars (are hard-coded as empty)" | ||
| - [ ] `prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/SlidingWindow.java:18` - "synchronized implementation, room for optimization" | ||
| - [ ] `prometheus-metrics-config/src/main/java/io/prometheus/metrics/config/PrometheusPropertiesLoader.java:105` - "Add environment variables like EXEMPLARS_ENABLED" | ||
| - [ ] `prometheus-metrics-exporter-opentelemetry/src/main/java/io/prometheus/metrics/exporter/opentelemetry/PrometheusMetricProducer.java:44` - "filter configuration for OpenTelemetry exporter" | ||
| - [ ] `prometheus-metrics-config/src/main/java/io/prometheus/metrics/config/ExporterOpenTelemetryProperties.java:7` - "JavaDoc missing" | ||
|
|
||
| ### 4. Improve Exception Handling | ||
| Replace broad `catch (Exception e)` with specific exception types: | ||
| - [ ] `prometheus-metrics-instrumentation-dropwizard5/src/main/java/.../DropwizardExports.java:237` | ||
| - [ ] `prometheus-metrics-instrumentation-caffeine/src/main/java/.../CacheMetricsCollector.java:229` | ||
| - [ ] `prometheus-metrics-exporter-opentelemetry/src/main/java/.../PrometheusInstrumentationScope.java:47` | ||
| - [ ] `prometheus-metrics-exporter-opentelemetry/src/main/java/.../OtelAutoConfig.java:115` | ||
| - [ ] `prometheus-metrics-instrumentation-jvm/src/main/java/.../JvmNativeMemoryMetrics.java:166` | ||
| - [ ] `prometheus-metrics-exporter-httpserver/src/main/java/.../HttpExchangeAdapter.java:115` | ||
|
|
||
| ## Medium Priority | ||
|
|
||
| ### 5. Add Branch Coverage to JaCoCo | ||
| - [ ] Update `pom.xml` to add branch coverage requirement (~50% minimum) | ||
| ```xml | ||
| <limit> | ||
| <counter>BRANCH</counter> | ||
| <value>COVEREDRATIO</value> | ||
| <minimum>0.50</minimum> | ||
| </limit> | ||
| ``` | ||
|
|
||
| ### 6. Raise Minimum Coverage Thresholds | ||
| Current thresholds to review: | ||
| - [ ] `prometheus-metrics-exporter-httpserver` - 45% (raise to 60%) | ||
| - [ ] `prometheus-metrics-instrumentation-dropwizard5` - 50% (raise to 60%) | ||
| - [ ] `prometheus-metrics-exposition-textformats` - 50% (raise to 60%) | ||
| - [ ] `prometheus-metrics-instrumentation-jvm` - 55% (raise to 60%) | ||
|
|
||
| ### 7. Add SpotBugs | ||
| - [ ] Add `spotbugs-maven-plugin` to `pom.xml` | ||
| - [ ] Configure with appropriate rule set | ||
|
|
||
| ### 8. Narrow Checkstyle Suppressions | ||
| - [ ] Review `checkstyle-suppressions.xml` - currently suppresses ALL Javadoc checks globally | ||
| - [ ] Narrow to specific packages/classes that need exceptions | ||
|
|
||
| ## Lower Priority | ||
|
|
||
| ### 9. Refactor Large Classes | ||
| - [ ] `prometheus-metrics-core/src/main/java/.../Histogram.java` (978 lines) - consider extracting native histogram logic | ||
|
|
||
| ### 10. Document Configuration Classes | ||
| - [ ] `PrometheusPropertiesLoader` - add JavaDoc | ||
| - [ ] `ExporterProperties` and related classes - add JavaDoc | ||
| - [ ] `ExporterOpenTelemetryProperties` - add JavaDoc (noted in TODO) | ||
|
|
||
| ### 11. Consolidate Servlet Exporter Duplication | ||
| - [ ] Extract common logic from `servlet-jakarta` and `servlet-javax` into `exporter-common` | ||
|
|
||
| ### 12. Add Mutation Testing | ||
| - [ ] Add Pitest (`pitest-maven`) for critical modules | ||
| - [ ] Start with `prometheus-metrics-core` and `prometheus-metrics-model` | ||
|
|
||
| --- | ||
|
|
||
| ## Progress Notes | ||
|
|
||
| _Add notes here as items are completed:_ | ||
|
|
||
| | Date | Item | Notes | | ||
| |------|------|-------| | ||
| | 2026-01-24 | Missing Test Coverage for Exporter Modules | Added 55 tests across 4 modules: exporter-common (22 tests), servlet-jakarta (14 tests), servlet-javax (14 tests), otel-agent-resources (5 tests). All tests passing. | | ||
| | 2026-01-24 | Eliminate Dropwizard Module Duplication | Created AbstractDropwizardExports base class (267 lines) with generic type parameters. Reduced dropwizard module from 297 to 209 lines (-88 lines, -30%), dropwizard5 module from 297 to 212 lines (-85 lines, -29%). All tests passing (32 tests dropwizard5, 13 tests dropwizard). | | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
120 changes: 120 additions & 0 deletions
120
...common/src/test/java/io/prometheus/metrics/exporter/common/PrometheusHttpRequestTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| package io.prometheus.metrics.exporter.common; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| import java.util.Collections; | ||
| import java.util.Enumeration; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class PrometheusHttpRequestTest { | ||
|
|
||
| @Test | ||
| public void testGetHeaderReturnsFirstValue() { | ||
| PrometheusHttpRequest request = | ||
| new TestPrometheusHttpRequest("name[]=metric1&name[]=metric2", "gzip"); | ||
| assertThat(request.getHeader("Accept-Encoding")).isEqualTo("gzip"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetHeaderReturnsNullWhenNoHeaders() { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i dont think these tests need to be public?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| PrometheusHttpRequest request = new TestPrometheusHttpRequest("", null); | ||
| assertThat(request.getHeader("Accept-Encoding")).isNull(); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetParameterReturnsFirstValue() { | ||
| PrometheusHttpRequest request = new TestPrometheusHttpRequest("name[]=metric1&name[]=metric2"); | ||
| assertThat(request.getParameter("name[]")).isEqualTo("metric1"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetParameterReturnsNullWhenNotPresent() { | ||
| PrometheusHttpRequest request = new TestPrometheusHttpRequest("other=value"); | ||
| assertThat(request.getParameter("name[]")).isNull(); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetParameterValuesReturnsMultipleValues() { | ||
| PrometheusHttpRequest request = new TestPrometheusHttpRequest("name[]=metric1&name[]=metric2"); | ||
| String[] values = request.getParameterValues("name[]"); | ||
| assertThat(values).containsExactly("metric1", "metric2"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetParameterValuesReturnsNullWhenNotPresent() { | ||
| PrometheusHttpRequest request = new TestPrometheusHttpRequest("other=value"); | ||
| assertThat(request.getParameterValues("name[]")).isNull(); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetParameterValuesWithEmptyQueryString() { | ||
| PrometheusHttpRequest request = new TestPrometheusHttpRequest(""); | ||
| assertThat(request.getParameterValues("name[]")).isNull(); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetParameterValuesWithNullQueryString() { | ||
| PrometheusHttpRequest request = new TestPrometheusHttpRequest(null); | ||
| assertThat(request.getParameterValues("name[]")).isNull(); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetParameterValuesWithUrlEncodedValues() { | ||
| PrometheusHttpRequest request = new TestPrometheusHttpRequest("name=hello%20world"); | ||
| String[] values = request.getParameterValues("name"); | ||
| assertThat(values).containsExactly("hello world"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetParameterValuesWithSpecialCharacters() { | ||
| PrometheusHttpRequest request = new TestPrometheusHttpRequest("name=%2Ffoo%2Fbar"); | ||
| String[] values = request.getParameterValues("name"); | ||
| assertThat(values).containsExactly("/foo/bar"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetParameterValuesIgnoresParametersWithoutEquals() { | ||
| PrometheusHttpRequest request = | ||
| new TestPrometheusHttpRequest("name[]=value1&invalid&name[]=value2"); | ||
| String[] values = request.getParameterValues("name[]"); | ||
| assertThat(values).containsExactly("value1", "value2"); | ||
| } | ||
|
|
||
| /** Test implementation of PrometheusHttpRequest for testing default methods. */ | ||
| private static class TestPrometheusHttpRequest implements PrometheusHttpRequest { | ||
| private final String queryString; | ||
| private final String acceptEncoding; | ||
|
|
||
| TestPrometheusHttpRequest(String queryString) { | ||
| this(queryString, null); | ||
| } | ||
|
|
||
| TestPrometheusHttpRequest(String queryString, String acceptEncoding) { | ||
| this.queryString = queryString; | ||
| this.acceptEncoding = acceptEncoding; | ||
| } | ||
|
|
||
| @Override | ||
| public String getQueryString() { | ||
| return queryString; | ||
| } | ||
|
|
||
| @Override | ||
| public Enumeration<String> getHeaders(String name) { | ||
| if (acceptEncoding != null && name.equals("Accept-Encoding")) { | ||
| return Collections.enumeration(Collections.singletonList(acceptEncoding)); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public String getMethod() { | ||
| return "GET"; | ||
| } | ||
|
|
||
| @Override | ||
| public String getRequestPath() { | ||
| return "/metrics"; | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we be committing these types of files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably better as an issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1816
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed in #1818