Closes #268 - Parse the agent configuration based on the MIME type when fetching it via HTTP

[heiko-holz]

Closes #268

---

This change is 

[mariusoe]

A general question and idea: since YAML is a superset of Json and therefore every Json object is also a valid YAML object, we don't even have to try to parse the configuration as a Json object. We can directly or always parse the object as YAML, since this will then always work. This also means that most of your code is obsolete (sorry for this :) ) and we only need to replace or remove the previously used call PropertyUtils.readJson(rawProperties);.

This also reduces complexity which is a good thing.

What do you think?

Reviewed 5 of 11 files at r1, all commit messages.
Reviewable status: 5 of 11 files reviewed, all discussions resolved (waiting on @mariusoe)

[heiko-holz]

Valid suggestion!

YamlPropertiesFactoryBean can also read JSON, but only if all keys are quoted.
Thus, if we assign the MIME type as application/x-yaml, but supply JSON without quoted keys, it will result in an invalid configuration, but no exception is thrown

Reviewable status: 5 of 11 files reviewed, all discussions resolved (waiting on @mariusoe)

[mariusoe]

Ok, but then how does it work currently? I mean, the configuration server is currently returning the configuration in the YAML syntax where some of the properties are using a JSON notation (without quoted keys).

Example:

 ...
      a_test:
        input: {_arg1: sample.Request}

Isn't the input object a JSON one?

Reviewable status: 5 of 11 files reviewed, all discussions resolved (waiting on @mariusoe)

[heiko-holz]

I found an example where the PropertyUtils.readYaml does not work as expected:

String json = "{a_test: {input: {_arg1: sample.Request}, array: [{x:42}, {y:1337}]}}";
Properties prop = PropertyUtils.readYaml(json); 

results in {a_test.array[0].x:42=, a_test.array[1].y:1337=, a_test.input._arg1=sample.Request}.
The values in the array are not correctly deserialized.

// quote the keys in the array
String jsonWithQuotedKeys= "{a_test: {input: {_arg1: sample.Request}}, array: [{\"x\":42}, {\"y\":1337}]}";
Properties propQ = PropertyUtils.readYaml(jsonWithQuotedKeys); 

results in {a_test.array[1].y=1337, a_test.input._arg1=sample.Request, a_test.array[0].x=42}, where the values in the array are correctly deserialized.

[heiko-holz]

/edit: I found a similar example in PropertyUtilsTest#json

[heiko-holz]

Apparently, this issue is not related to JSON strings but in general to arrays in yaml Strings:

// keys in array not quoted
String noQuotedKeysInArray = "a_test:\n  input:\n    arr: [\n   x:1,\n  y:2\n]";
System.out.printf(PropertyUtils.readYaml(noQuotedKeysInArray).toString());

results in {a_test.input.arr[1]=y:2, a_test.input.arr[0]=x:1}, the objects in the array are not correctly deserialized

// keys in array are quoted
String withQuotedKeysInArray = "a_test:\n  input:\n    arr: [\"x\":1,\"y\":2]";
System.out.printf(PropertyUtils.readYaml(withQuotedKeysInArray).toString());

results in {a_test.input.arr[1].y=2, a_test.input.arr[0].x=1}, the objects in the array are correctly deserialized

[heiko-holz]

When using pure YAML notation, we do not need any quotation marks for the keys.

E.g., the following String can be properly parsed:

String yamlList = "a_test:\n  input:\n      arr:\n      - x: 1\n      - y: 2\n";
Properties result = PropertyUtils.readYaml(yamlList);

 System.out.println(result.toString());

results in {a_test.input.arr[1].y=2, a_test.input.arr[0].x=1}

Reviewable status: 5 of 11 files reviewed, all discussions resolved (waiting on @mariusoe)

[codecov]

Codecov Report

Merging #1251 (9e0732d) into master (85b5e4a) will decrease coverage by 0.05%.
The diff coverage is 77.42%.

@@             Coverage Diff              @@
##             master    #1251      +/-   ##
============================================
- Coverage     81.07%   81.01%   -0.05%     
- Complexity     2069     2070       +1     
============================================
  Files           210      211       +1     
  Lines          6591     6593       +2     
  Branches        784      782       -2     
============================================
- Hits           5343     5341       -2     
- Misses          949      951       +2     
- Partials        299      301       +2     

Impacted Files	Coverage Δ
.../propertysources/file/DirectoryPropertySource.java	89.80% <0.00%> (-0.40%)	⬇️
...ectit/ocelot/core/config/InspectitEnvironment.java	75.19% <37.50%> (-2.76%)	⬇️
...spectit/ocelot/core/config/util/PropertyUtils.java	91.30% <92.86%> (-4.35%)	⬇️
...tysources/file/ConfigurationDirectoriesPoller.java	90.62% <100.00%> (ø)
.../propertysources/http/HttpPropertySourceState.java	77.60% <100.00%> (ø)
...t/core/config/util/InvalidPropertiesException.java	100.00% <100.00%> (ø)
...nspectit/ocelot/core/utils/HighPrecisionTimer.java	89.06% <0.00%> (-1.56%)	⬇️
.../ocelot/core/metrics/system/GCMetricsRecorder.java	72.13% <0.00%> (-0.82%)	⬇️
... and 1 more

[mariusoe]

Reviewed 2 of 11 files at r1, 6 of 10 files at r2, 9 of 9 files at r3, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @heiko-holz)

---

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/command/AgentCommandService.java, line 109 at r3 (raw file):

            }

            String urlBase = String.format("%s://%s", url.getProtocol(), url.getHost());

Do we need to verify that this is not null as well? Could this happen at all?

Code quote:

url.getProtocol()

---

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/config/InspectitEnvironment.java, line 295 at r3 (raw file):

            Properties properties = null;
            try {
                properties = PropertyUtils.readYamlFiles(res);

Question: is it better that we have no configurationwhen a single file is invalid, or to have a partial configuration which is only a subset of the desired one because only the valid YAML files have been parsed?

I'm not sure which is better. What do you think?

---

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/config/propertysources/http/HttpPropertySourceState.java, line 123 at r3 (raw file):

    public boolean update(boolean fallBackToFile) {
        String configuration = fetchConfiguration(fallBackToFile);
        log.info("configuration={}", configuration);

Log statement is not needed.

---

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/config/util/InvalidPropertiesException.java, line 3 at r3 (raw file):

package rocks.inspectit.ocelot.core.config.util;

public class InvalidPropertiesException extends Exception {

Please add some comments to the class itself.

---

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/config/util/PropertyUtils.java, line 61 at r3 (raw file):

     * @return the generated {@link Properties} object
     */
    public static Properties readYamlOrJson(String yamlOrJson) throws InvalidPropertiesException {

How about naming this just readYaml so that we have a similar version of the readYamlFiles with this one?

---

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/config/propertysources/file/DirectoryPropertySourceIntTest.java, line 25 at r3 (raw file):

    private static final String tmpDir = "tmpconf_dirprop_test";

using File makes it easier and you don't have to create new file instance all the time :)

Code quote:

String

---

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/config/propertysources/file/DirectoryPropertySourceIntTest.java, line 28 at r3 (raw file):

    @BeforeAll
    static void createConfigDir() {
        new File(tmpDir).mkdir();

Imo it is a cleaner way to use the Java temp directory for this: Files.createTempDirectory(prefix);
Thus, in case the test exists or is getting killed, the folders do not clutter up the current working directory :)
And it may be very unlikely, but if by chance there should be a folder named "tmpconf_dirprop_test" in the working directory, it would be deleted.

See DirectoryPropertySourceTest

---

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/config/propertysources/file/DirectoryPropertySourceTest.java, line 74 at r3 (raw file):

    @DirtiesContext
    @Nested
    @ExtendWith(MockitoExtension.class)

Annotation can be put on outer class

---

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/config/propertysources/file/DirectoryPropertySourceTest.java, line 118 at r3 (raw file):

    @Nested
    @DirtiesContext
    @ExtendWith(MockitoExtension.class)

Annotation can be put on outer class

---

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/config/util/PropertyUtilsTest.java, line 13 at r3 (raw file):

public class PropertyUtilsTest {

    String json = "{\"arr\": [{\"x\":42},{\"y\":7},[\"A\",\"B\"]],nested:{\"name\":\"blub\",\"complex.key\":true}}";

Imo it is a better style to have test specific fields in the test classes where they are used. E.g. this field can be moved into the ReadJson class because it is only used there and not accross multiple nested test classes.

---

inspectit-ocelot-documentation/docs/breaking-changes/breaking-changes.md, line 6 at r3 (raw file):

---

## Breaking changes in <mark>1.15.X</mark>

Maybe you can use something like "CHANGEME" where it is clear in any case that it needs to be adapted

Code quote:

<mark>1.15.X</mark>