Remove state provider #39

bonndan · 2019-10-26T08:21:12Z

This implements #37

I don't expect to you to fully understand the logic, it is sufficient to review the code for mistakes.

jpgoelz

While I cannot yet comprehend the logic, I also cannot find any obvious mistakes either.

I ran the program and it worked. I read all the files and checked the file changes. Anything, in particular, I should have been looking for?

bonndan · 2019-10-28T13:04:37Z

There is an example File containing rancher1 Prometheus Export Data. I have served that file through nginx and it worked as expected. No more to do here

Matthimatiker

Looks reasonable, although I do not understand all the details 👍

The code contains an alarming number of manual type casts, which you might get rid of by improving the signatures with generic method parameters.

Items contains a lot of static methods. Breaking these up code improve readability. E.g.

var filtered = Items.of(landscape).findAll(criteria);

By returning Items, chaining would be possible and simplify working with subsets:

 var filtered = Items.of(landscape).findAll(criteria).findAll(anotherCriteria);

Eventually, Items might evolve into type-safe collection/query engine for Landscape items.

Matthimatiker · 2019-10-28T09:33:22Z

src/main/java/de/bonndan/nivio/input/dto/SourceFormat.java

+    private static final List<SourceFormat> KNOWN_FORMATS;
+
+    static {
+        KNOWN_FORMATS = Arrays.asList(NIVIO, DOCKER_COMPOSE2, KUBERNETES, RANCHER1_PROMETHEUS);
+    }


Isn't assignment on same line just the same, only simpler to read?

private static final List<SourceFormat> KNOWN_FORMATS = List.of(NIVIO, DOCKER_COMPOSE2, KUBERNETES, RANCHER1_PROMETHEUS);

The current implementation is soon obsolete. #14 will replace it with something happening at runtime.

Matthimatiker · 2019-10-28T17:55:49Z

src/main/java/de/bonndan/nivio/input/rancher1/ItemDescriptionFactoryRancher1.java

+        } catch (MalformedURLException e) {
+            logger.error("Could not work on prometheus url {}", combine);
+            return new ArrayList<>();
+        }


Should that happen? If that's a configuration error, it might be helpful to simply forward the exception (e.g. wrapping it into an IllegalStateException).

Matthimatiker · 2019-10-28T17:59:54Z

src/main/java/de/bonndan/nivio/input/rancher1/PrometheusExporter.java

+                        .forEach(itemDescription -> {
+                            ItemDescription inMap = tmp.computeIfAbsent(itemDescription.getFullyQualifiedIdentifier(), ItemDescription::new);
+                            ItemDescriptionFactory.assignNotNull(inMap, itemDescription);
+                        });
            });

        } catch (IOException e) {
            logger.error("Failed to scrape " + target, e);


This could be anything, e.g. a full hard drive. Wrap and re-throw might be a better option here.

Matthimatiker · 2019-10-28T18:02:05Z

src/main/java/de/bonndan/nivio/input/rancher1/PrometheusExporter.java

        PrometheusScraper prometheusScraper = getScraper();
-        final Map<FullyQualifiedIdentifier, StatusItem> tmp = new HashMap<>();
+        final Map<FullyQualifiedIdentifier, ItemDescription> tmp = new HashMap<>();


Is this some sort of cache?

No, not intentionally. This state was necessary before when the class was responsible for collecting statuses only. The whole class could be merged with the ItemDescriptionFactory for rancher/prometheus.

Matthimatiker · 2019-10-28T18:03:48Z

src/main/java/de/bonndan/nivio/input/rancher1/PrometheusExporter.java

            if (metric.getLabels().getOrDefault("health_state", "").equals("unhealthy") && metric.getValue() > 0) {
-                return new StatusDescription(StatusItem.HEALTH, Status.ORANGE, metric.getLabels().getOrDefault("health_state", ""));
+                health_state = new StatusDescription(StatusItem.HEALTH, Status.ORANGE, metric.getLabels().getOrDefault("health_state", ""));


Perhaps you should stick with camelCase here.

src/test/java/de/bonndan/nivio/input/rancher1/PrometheusExporterTest.java

src/main/java/de/bonndan/nivio/model/Items.java

Matthimatiker · 2019-10-28T18:21:15Z

src/test/java/de/bonndan/nivio/input/rancher1/PrometheusExporterTest.java

+    public void testSuccess() throws MalformedURLException {
+
+        String path = RootPath.get() + "/src/test/resources/example/rancher_prometheus_exporter.txt";
+        String prometheusExport = FileFetcher.readFile(new File(path));


You might want to read resources files from the class path. Most probably, this implementation depends on the working directory.

A construct like the following should work (I do not remember all the details):

getClass().getResourceAsInputStream("/example/rancher_prometheus_exporter.txt"); // convert this into a string

bonndan · 2019-10-28T19:40:06Z

Looks reasonable, although I do not understand all the details

The code contains an alarming number of manual type casts, which you might get rid of by improving the signatures with generic method parameters.

Items contains a lot of static methods. Breaking these up code improve readability. E.g.
var filtered = Items.of(landscape).findAll(criteria);
By returning Items, chaining would be possible and simplify working with subsets:
 var filtered = Items.of(landscape).findAll(criteria).findAll(anotherCriteria);
Eventually, Items might evolve into type-safe collection/query engine for Landscape items.

Yes, a lot of code can be refactored, since some of it was used when JPA persistence was in place.

Daniel Pozzi added 3 commits October 26, 2019 10:07

[#37] removed state providers

c0fa7c1

[#37] updated docs

3c13050

int test example

1ebd7c9

bonndan assigned Matthimatiker and jpgoelz Oct 26, 2019

bonndan added this to In progress in nivio via automation Oct 26, 2019

bonndan added this to the 0.2 milestone Oct 26, 2019

bonndan unassigned Matthimatiker and jpgoelz Oct 26, 2019

bonndan requested review from jpgoelz and Matthimatiker October 26, 2019 08:21

jpgoelz reviewed Oct 28, 2019

View reviewed changes

bonndan merged commit 8999121 into develop Oct 28, 2019

nivio automation moved this from In progress to Done Oct 28, 2019

bonndan deleted the remove_state_provider branch October 28, 2019 15:28

nivio automation moved this from Done to Reviewer approved Oct 28, 2019

Matthimatiker approved these changes Oct 28, 2019

View reviewed changes

bonndan mentioned this pull request Oct 28, 2019

Get rid of several type casts by improving the type definition #40

Closed

bonndan moved this from Reviewer approved to Done in nivio Nov 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove state provider #39

Remove state provider #39

bonndan commented Oct 26, 2019

jpgoelz left a comment •

edited

bonndan commented Oct 28, 2019 •

edited

Matthimatiker left a comment

Matthimatiker Oct 28, 2019

bonndan Oct 28, 2019

Matthimatiker Oct 28, 2019

Matthimatiker Oct 28, 2019

Matthimatiker Oct 28, 2019

bonndan Oct 28, 2019

Matthimatiker Oct 28, 2019

Matthimatiker Oct 28, 2019

bonndan commented Oct 28, 2019

Remove state provider #39

Remove state provider #39

Conversation

bonndan commented Oct 26, 2019

jpgoelz left a comment • edited

Choose a reason for hiding this comment

bonndan commented Oct 28, 2019 • edited

Matthimatiker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonndan commented Oct 28, 2019

jpgoelz left a comment •

edited

bonndan commented Oct 28, 2019 •

edited