Skip to content
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

Prometheus Exemplars are missing from _count #40904

Closed
jonatan-ivanov opened this issue May 25, 2024 · 7 comments
Closed

Prometheus Exemplars are missing from _count #40904

jonatan-ivanov opened this issue May 25, 2024 · 7 comments
Assignees
Labels
type: regression A regression from a previous release
Milestone

Comments

@jonatan-ivanov
Copy link
Member

It seems that 3.3.0 introduced a regression during the migration of Prometheus 1.x: exemplars are missing from _count time series.

I think there are two bugs that are causing this:

  1. It seems PrometheusPropertiesConfigAdapter does not respect the Micrometer defaults for "Prometheus Properties" (these) when there are no user-defined custom properties set:
    private Properties fromPropertiesMap(PrometheusProperties prometheusProperties) {
    Map<String, String> map = prometheusProperties.getProperties();
    if (map.isEmpty()) {
    return null;
    }
    Properties properties = PrometheusConfig.super.prometheusProperties();
    properties = (properties != null) ? properties : new Properties();
    properties.putAll(map);
    return properties;
    }

    It seems Micrometer defaults (PrometheusConfig.super.prometheusProperties()) are only respected and merged with user-defined custom properties when they present, otherwise Micrometer defaults are ignored. This is a problem since Micrometer sets some defaults which can be ignored or not ignored depending on the presence of user-defined properties.
  2. Ignoring Micrometer defaults could have been worked around by manually setting them from user-properties (or by setting any custom Prometheus property) but another bug prevents them to take effect on the exporter. It seems PrometheusOutputFormat does not use these properties when it initializes the exporters:
    Since PrometheusOutputFormat calls ExpositionFormats.init() instead of ExpositionFormats.init(ExporterProperties), no property set by Micrometer/Boot/user can have any effect on the exporters since they are ignored (in comparison, Micrometer does this).

One workaround could be using the Prometheus property loading mechanism. Adding a prometheus.properties file to the classpath for example placing it to the resources folder with the following content brings exemplars on _count back:

io.prometheus.exporter.exemplarsOnAllMetricTypes=true

In order to reproduce it, you need to ask for the OpenMetrics format from Boot:

http :8080/actuator/prometheus 'Accept: application/openmetrics-text; version=1.0.0' | grep 'trace_id'

and you will need Micrometer's (1.13.x) Prometheus (1.x) registry and Micrometer Tracing: start.spring.io example

@jonatan-ivanov jonatan-ivanov added the status: waiting-for-triage An issue we've not yet triaged label May 25, 2024
@jonatan-ivanov jonatan-ivanov changed the title Exemplars are missing from _count Prometheus Exemplars are missing from _count May 25, 2024
@mhalbritter
Copy link
Contributor

mhalbritter commented May 27, 2024

Hello Jonatan,

for 1: fromPropertiesMap is used in

public Properties prometheusProperties() {
  return get(this::fromPropertiesMap, PrometheusConfig.super::prometheusProperties);
}

and this get method does this:

protected final <V> V get(Function<T, V> getter, Supplier<V> fallback) {
  V value = getter.apply(this.properties);
  return (value != null) ? value : fallback.get();
}

So if getter returns null, it invokes fallback.

With our invocation, if fromPropertiesMap returns null, it uses super.prometheusProperties().

Looking at fromPropertiesMap:

Map<String, String> map = prometheusProperties.getProperties();
if (map.isEmpty()) {
  return null;
}

So if there are no user-defined properties, we return null, and get will use super.prometheusProperties().

This looks fine to me, or have I overlooked something?

@mhalbritter mhalbritter added the status: waiting-for-feedback We need additional information before we can continue label May 27, 2024
@spring-projects-issues
Copy link
Collaborator

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@spring-projects-issues spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Jun 3, 2024
@spring-projects-issues
Copy link
Collaborator

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

@spring-projects-issues spring-projects-issues closed this as not planned Won't fix, can't repro, duplicate, stale Jun 10, 2024
@spring-projects-issues spring-projects-issues removed status: waiting-for-feedback We need additional information before we can continue status: feedback-reminder We've sent a reminder that we need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Jun 10, 2024
@mhalbritter mhalbritter self-assigned this Jun 10, 2024
@mhalbritter mhalbritter added the type: regression A regression from a previous release label Jun 10, 2024
@mhalbritter mhalbritter reopened this Jun 10, 2024
@mhalbritter
Copy link
Contributor

mhalbritter commented Jun 10, 2024

The bot reminded me that there's something pending. I gave it another look, and it looks like that point 1 that @jonatan-ivanov has raised isn't valid, but the 2nd is.

I have something in https://github.com/mhalbritter/spring-boot/tree/mh/40904-prometheus-exemplars-are-missing-from-_count, but I need to play around with it a bit more.

@mhalbritter mhalbritter added this to the 3.3.x milestone Jun 11, 2024
mhalbritter added a commit that referenced this issue Jun 11, 2024
@mhalbritter mhalbritter modified the milestones: 3.3.x, 3.3.1 Jun 11, 2024
@mhalbritter
Copy link
Contributor

I also added a smoke test to verify that it doesn't break in the future.

@jonatan-ivanov
Copy link
Member Author

Sorry for the late response, you are right about PrometheusProperties, I checked it in the ctor of PrometheusMeterRegistry, the property is there, sorry for the confusion.

I looked at your fix in PrometheusScrapeEndpoint, it looks great, I have a quite minor comment for this section:

public PrometheusScrapeEndpoint(PrometheusRegistry prometheusRegistry, Properties expositionFormatsProperties) {
this.prometheusRegistry = prometheusRegistry;
PrometheusProperties prometheusProperties = (expositionFormatsProperties != null)
? PrometheusPropertiesLoader.load(expositionFormatsProperties) : PrometheusPropertiesLoader.load();
this.expositionFormats = ExpositionFormats.init(prometheusProperties.getExporterProperties());
}

What do you think about renaming expositionFormatsProperties to exporterProperties since that's the name Prometheus itself uses for these properties, see prometheusProperties.getExporterProperties() and the docs.

I also checked the latest snapshots, it works, thank you very much!

@mhalbritter
Copy link
Contributor

Thanks for the feedback! Sounds good, will rename the parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: regression A regression from a previous release
Projects
None yet
Development

No branches or pull requests

3 participants