Skip to content

Builder.withoutExemplars() setting cannot be overridden via properties #1477

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cruftex
Copy link

@cruftex cruftex commented Jul 23, 2025

The PR will fix the behaviour that exemplar support can be switched on via the properties, although it was disabled explicitly in the code definition by withoutExemplars. Currently this would enable exemplars:

    PrometheusProperties properties = PrometheusProperties.builder()
      .putMetricProperty("count", MetricsProperties.builder()
        .exemplarsEnabled(true)
        .build())
      .build();
    Counter counter =
      Counter.builder(properties)
        .name("count_total")
        .withoutExemplars()
        .build();

In TDD fashion I will add failing tests first.

@cruftex cruftex force-pushed the exemplars_off branch 3 times, most recently from 3444294 to 3e792c9 Compare July 23, 2025 13:06
@cruftex cruftex changed the title Work in progress: withoutExemplars cannot be overridden via specific metric property Builder.withoutExemplars() setting cannot be overridden via properties Jul 23, 2025
@cruftex
Copy link
Author

cruftex commented Jul 23, 2025

@zeitlinger
This PR is now ready for review and merging.

Somehow the build is not starting. Is there something else I need to do?

@zeitlinger
Copy link
Member

Somehow the build is not starting. Is there something else I need to do?

no - that needs to be approved

@zeitlinger zeitlinger enabled auto-merge (squash) July 24, 2025 07:35
@zeitlinger
Copy link
Member

zeitlinger commented Jul 24, 2025

@cruftex the code is not formatted correctly - run ./mvnw spotless:apply

auto-merge was automatically disabled July 24, 2025 09:35

Head branch was pushed to by a user without write access

@cruftex
Copy link
Author

cruftex commented Jul 24, 2025

@zeitlinger sorry... done!

@zeitlinger zeitlinger enabled auto-merge (squash) July 24, 2025 11:56
@zeitlinger
Copy link
Member

now there's a checkstyle issue

[WARN] /home/runner/work/client_java/client_java/prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/Summary.java:52:5: Distance between variable 'exemplarsEnabled' declaration and its first usage is 4, but allowed 3. Consider making that variable final if you still need to store its value in advance (before method calls that might have side effects on the original value). [VariableDeclarationUsageDistance]
[WARN] /home/runner/work/client_java/client_java/prometheus-metrics-core/src/main/java/io/prometheus/metrics/core/metrics/Histogram.java:118:5: Distance between variable 'exemplarsEnabled' declaration and its first usage is 7, but allowed 3. Consider making that variable final if you still need to store its value in advance (before method calls that might have side effects on the original value). [VariableDeclarationUsageDistance]
Audit done.

…overridden via properties. Introduce builder for PrometheusProperties for testing. Minimal housekeeping, removing the redundant boolean flag.

Signed-off-by: Jens Wilke <[email protected]>
auto-merge was automatically disabled July 24, 2025 16:21

Head branch was pushed to by a user without write access

@cruftex
Copy link
Author

cruftex commented Jul 24, 2025

I agree with checkstyle. Corrected.

@cruftex
Copy link
Author

cruftex commented Jul 29, 2025

@zeitlinger ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants