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

Fix Describable returning an empty list #785

Merged
merged 2 commits into from
Jun 15, 2022
Merged

Fix Describable returning an empty list #785

merged 2 commits into from
Jun 15, 2022

Conversation

fstab
Copy link
Member

@fstab fstab commented May 19, 2022

As described in #780, if a Collector implements Describable, but the describe() method returns an empty list, the metrics from this Collector will never be collected.

This PR should fix this.

if (sampleNameFilter.test(entry.getKey())) {
collectors.add(entry.getValue());
for (Map.Entry<Collector, List<String>> entry : collectorsToNames.entrySet()) {
for (String name : entry.getValue()) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I am missing something but it doesn't seem to fix the issue.
The problem is the List<String> of names is empty, so this inner loop is not going to run.
One way to make it work, though I don't know if this is the behavior you want, is something like this:

          for (Map.Entry<Collector, List<String>> entry : collectorsToNames.entrySet()) {
            if (entry.getValue().isEmpty()) {
              collectors.add(entry.getKey());
            } else {
              for (String name : entry.getValue()) {
                if (sampleNameFilter.test(name)) {
                  collectors.add(entry.getKey());
                  break;
                }
              }
            }
          }

Signed-off-by: Fabian Stäber <fabian@fstab.de>
Signed-off-by: Fabian Stäber <fabian@fstab.de>
@fstab fstab merged commit 7de891e into master Jun 15, 2022
@fstab fstab deleted the fix-describable branch June 15, 2022 09:58
@fstab
Copy link
Member Author

fstab commented Jun 15, 2022

Thanks a lot @yaronel. I fixed it and added a test to make sure it works.

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.

None yet

2 participants