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

Accidental breaking API change #892

Closed
fstab opened this issue Nov 4, 2023 · 6 comments · Fixed by #898
Closed

Accidental breaking API change #892

fstab opened this issue Nov 4, 2023 · 6 comments · Fixed by #898

Comments

@fstab
Copy link
Member

fstab commented Nov 4, 2023

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
Copy link
Contributor

ganzuoni commented Nov 5, 2023

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
Copy link
Collaborator

dhoard commented Nov 6, 2023

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
Copy link
Contributor

ganzuoni commented Nov 7, 2023

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
Copy link
Collaborator

dhoard commented Nov 7, 2023

@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
Copy link
Contributor

ganzuoni commented Nov 8, 2023

@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 added a commit that referenced this issue Nov 12, 2023
fstab added a commit that referenced this issue Nov 12, 2023
Signed-off-by: Fabian Stäber <fabian@fstab.de>
fstab added a commit that referenced this issue Nov 12, 2023
Signed-off-by: Fabian Stäber <fabian@fstab.de>
@fstab
Copy link
Member Author

fstab commented Nov 12, 2023

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.

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 a pull request may close this issue.

3 participants