Closes #1049 - Edit the 'illegal tag value' error message to be printed more than once #1064

Mahir-Isikli · 2021-05-01T17:33:28Z

This change is

Patrick-Eichhorn

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @Mahir-Isikli and @Patrick-Eichhorn)

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/tags/TagUtils.java, line 12 at r4 (raw file):

    private static int printedWarningCounter = 0;

    private static int maxWarningPrints = 10;

Does not have to be static and can be final

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/tags/TagUtils.java, line 16 at r4 (raw file):

    private static long lastWarningTime = 0;

    private static int waitingTimeInMinutes = 10;

Does not have to be static and can be final
I think we can replace the minutes to seconds to allow a finer adjustment.

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/tags/TagUtilsTest.java, line 27 at r4 (raw file):

        assertThat(TagUtils.createTagValue("my-tag-key", "non-printable-character-\u007f")).isEqualTo(TagValue.create("<invalid>"));
    }

I think the tests do not work because the "printedWarningCounter" is static and already incremented by the previous tests (createTagValue_tooLong() and createTagValue_nonPrintableCharacter()).
For this case you can make the printedWarningCounter public (package) and reset it to 0 for this test.
For such a case, we use the @VisibleForTesting annotation for the variable to make it clear why it is public.

Mahir-Isikli

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @Patrick-Eichhorn and @VisibleForTesting)

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/tags/TagUtils.java, line 12 at r4 (raw file):

Previously, Patrick-Eichhorn wrote…

Does not have to be static and can be final

Done.

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/tags/TagUtils.java, line 16 at r4 (raw file):

Previously, Patrick-Eichhorn wrote…

Does not have to be static and can be final
I think we can replace the minutes to seconds to allow a finer adjustment.

Done.

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/tags/TagUtilsTest.java, line 27 at r4 (raw file):

Previously, Patrick-Eichhorn wrote…

I think the tests do not work because the "printedWarningCounter" is static and already incremented by the previous tests (createTagValue_tooLong() and createTagValue_nonPrintableCharacter()).
For this case you can make the printedWarningCounter public (package) and reset it to 0 for this test.
For such a case, we use the @VisibleForTesting annotation for the variable to make it clear why it is public.

Done.

mariusoe

Good job Mahir. :)

I have a few more minor comments that you can take a look at. I put the im the EUM project, but they also refer to the one in the core project.
Once these are fixed, we can merge the PR.

Reviewed 1 of 2 files at r3, 3 of 3 files at r5.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Mahir-Isikli, @Patrick-Eichhorn, and @VisibleForTesting)

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/utils/TagUtils.java, line 12 at r5 (raw file):

    @VisibleForTesting
    static int printedWarningCounter = 0;

Please add some comments to all static fields.

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/utils/TagUtils.java, line 17 at r5 (raw file):

    static long lastWarningTime = 0;

    private final static int maxWarningPrints = 10;

It is common pratices that constant fields are written in uppcer cases: MAX_WARNING_PRINTS

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/utils/TagUtils.java, line 52 at r5 (raw file):

            printedWarningCounter++;
            if (printedWarningCounter == maxWarningPrints) {
                lastWarningTime = System.currentTimeMillis();

We should print a log message, that further log messages are suppressed, so that the user knows why no more logs appear.

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/utils/TagUtils.java, line 55 at r5 (raw file):

            }

        } else if ((System.currentTimeMillis() - lastWarningTime) > waitingTimeInMilliSeconds) {

We should reset this counter also if the last warning was more than waitingTimeInMilliSeconds ago and not only if the counter has reached the maxWarningPrints value.

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/utils/TagUtils.java, line 57 at r5 (raw file):

        } else if ((System.currentTimeMillis() - lastWarningTime) > waitingTimeInMilliSeconds) {
            printedWarningCounter = 0;
            printWarning(tagKey, value);

Can your restructure this method a bit, so that the method does not have to call itself? This would make them a little easier to understand.

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/SpringTestBase.java, line 151 at r5 (raw file):

Integer

You can just use primitives instead of their wrapper classes: int

Mahir-Isikli

Reviewable status: 2 of 4 files reviewed, 9 unresolved discussions (waiting on @Mahir-Isikli, @mariusoe, @Patrick-Eichhorn, and @VisibleForTesting)

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/utils/TagUtils.java, line 12 at r5 (raw file):