Accidental breaking API change

[fstab]

With #873 we accidentally introduced a breaking API change. Before the change, you could pass a metric name filter as a lambda expression:

registry.scrape(name -> name.equals("my_metric"));

Now, this fails:

reference to scrape is ambiguous
[ERROR]   both method scrape(io.prometheus.metrics.model.registry.PrometheusScrapeRequest) in io.prometheus.metrics.model.registry.PrometheusRegistry and method scrape(java.util.function.Predicate<java.lang.String>) in io.prometheus.metrics.model.registry.PrometheusRegistry match

Same for (Multi)Collector.collect(name -> name.equals("...")).

@ganzuoni what do you think is a good way to fix this? I know your initial PR had a method getRequestPath(), which we removed because it wasn't needed. I guess we could add it again to the PrometheusScrapeRequest interface? What do you think?

[ganzuoni]

I guess, it will not solve the issue that is caused by method overloading resolution with the nastiest (IMHO) "innovation" ever introduced in Java ecosystem: lambda expression..
You will get the same error if you have a statement like
registry.scrape(null)

Compiler cannot select the right method in this case because null is null and there's no way to distinguish a null Predicate from a null PrometheusScrapeRequest
If you give a hint to the compiler it works

registry.scrape((Predicate<String>) null)

in the same way this statement works as expected

registry.scrape((Predicate<String>) name -> name.equals("my_metric"));

Other solutions require different method names e.g.,
filteredScrape(Predicate<String>)
or
advancedScrape(PrometheusScrapeRequest)

[dhoard]

Having two methods seems like the wrong approach.

What about...

registry.scrape(Predicate<PrometheusScrapeRequest>)

Then you can implement...

public class PrometheusNamePredicate implements Predicate<PrometheusScrapeRequest> {

    private Set<String> includeNames;

    public PrometheusNamePredicate(Set<String> includeNames) {
        this.includeNames = includeNames;
    }

    public MetricSnapshot test(PrometheusScrapeRequest PrometheusScrapeRequest) {
        MetricSnapshot result = collect(scrapeRequest);
        if (includedNames.contains(result.getMetadata().getPrometheusName())) {
            return result;
        } else {
            return null;
        }
    }
}

public class MetricNamePredicate implements Predicate<PrometheusScrapeRequest> {

    private Set<String> includeNames;

    public MetricNamePredicate(Set<String> includeNames) {
        this.includeNames = includeNames;
    }

    public MetricSnapshot test(PrometheusScrapeRequest PrometheusScrapeRequest) {
        MetricSnapshot result = collect(scrapeRequest);
        if (includedNames.contains(result.getMetadata().getName())) {
            return result;
        } else {
            return null;
        }
    }
}

I believe this would also work with a lambda.

[ganzuoni]

Why 2 methods are a wrong approach ?
(https://blog.jooq.org/you-will-regret-applying-overloading-with-lambdas/)

My comment in the code

public class MetricNamePredicate implements Predicate<PrometheusScrapeRequest> {

    private Set<String> includeNames;

    public MetricNamePredicate(Set<String> includeNames) {
        this.includeNames = includeNames;
    }

    public MetricSnapshot test(PrometheusScrapeRequest PrometheusScrapeRequest) {
        MetricSnapshot result = collect(scrapeRequest); // **what is the class implementing collect(scrapeRequest) ?**
        if (includedNames.contains(result.getMetadata().getName())) {
            return result;
        } else {
            return null;
        }
    }
}

[dhoard]

@ganzuoni you're correct, the code is broken (that's what I get for trying to do it on the fly/from my phone.)

[ganzuoni]

@dhoard my point was not to highlight the missing method, but to underline that a predicate, whose job is simple say yes/no to an item, should instead acquire the metrics from the Registry/Collector. More, in the MultiCollector case it would not work at all.
@fstab what's your opinion?

[fstab]

I merged #898 and restored the getRequestPath() method. As @ganzuoni pointed out that does not fix registry.scrape(null), but I can't think of any reasonable scenario where you would call scrape(null), so I think it's ok.

The most common case, registry.scrape(name -> name.equals("my_metric")) is fixed by restoring getRequestPath().

The multi target pattern is not a common use case, while filtering by metric name is common. So it's reasonable to have a simple lambda expression Predicate<String> for filtering by metric name, and an actual interface PrometheusScrapeRequest for the multi target pattern.