Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closes #966 - Resolve 'rt.bmr' data send by Boomerang plugin RT #970

Merged

Conversation

Patrick-Eichhorn
Copy link
Contributor

@Patrick-Eichhorn Patrick-Eichhorn commented Jan 18, 2021

This change is Reviewable

mariusoe
mariusoe previously approved these changes Jan 19, 2021
Copy link
Member

@mariusoe mariusoe left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Patrick-Eichhorn)


components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/beacon/processor/CsvExpanderBeaconProcessor.java, line 32 at r1 (raw file):

targetAttribute != null && !targetAttribute.equals("")

Instead of this, you can also use the following method of Apache Commons:

StringUtils.isNotBlank(targetAttribute)

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/beacon/processor/CsvExpanderBeaconProcessor.java, line 40 at r1 (raw file):

                beacon = beacon.merge(Collections.singletonMap(resultKey, attributes[i]));

                sum += Integer.parseInt(attributes[i]);

We should wrap this in a try/catch block, in case the number is not a valid number. In that case, this would stop the processing of the bmr Attribute.


components/inspectit-ocelot-eum-server/src/test/java/rocks/inspectit/oce/eum/server/beacon/processor/CsvExpanderBeaconProcessorTest.java, line 23 at r1 (raw file):

    @Nested
    public class Process {

Can you add three more tests to check whats happening when the bmr attribute is like:

  • 1,,1
    • I would expect: bmr.0 = 1,bmr.2 = 1, bmr.sum = 2
  • 1,not a number,1
    • I would expect: bmr.0 = 1,bmr.2 = 1, bmr.sum = 2
  • abc
    • I would expect: bmr.0 and bmr.sum does not exist

Copy link
Contributor Author

@Patrick-Eichhorn Patrick-Eichhorn left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 4 files reviewed, 3 unresolved discussions (waiting on @mariusoe)


components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/beacon/processor/CsvExpanderBeaconProcessor.java, line 32 at r1 (raw file):

Previously, mariusoe (Marius Oehler) wrote…
targetAttribute != null && !targetAttribute.equals("")

Instead of this, you can also use the following method of Apache Commons:

StringUtils.isNotBlank(targetAttribute)

Done.


components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/beacon/processor/CsvExpanderBeaconProcessor.java, line 40 at r1 (raw file):

Previously, mariusoe (Marius Oehler) wrote…

We should wrap this in a try/catch block, in case the number is not a valid number. In that case, this would stop the processing of the bmr Attribute.

Done.


components/inspectit-ocelot-eum-server/src/test/java/rocks/inspectit/oce/eum/server/beacon/processor/CsvExpanderBeaconProcessorTest.java, line 23 at r1 (raw file):

Previously, mariusoe (Marius Oehler) wrote…

Can you add three more tests to check whats happening when the bmr attribute is like:

  • 1,,1
    • I would expect: bmr.0 = 1,bmr.2 = 1, bmr.sum = 2
  • 1,not a number,1
    • I would expect: bmr.0 = 1,bmr.2 = 1, bmr.sum = 2
  • abc
    • I would expect: bmr.0 and bmr.sum does not exist

Done.

Copy link
Member

@mariusoe mariusoe left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Patrick-Eichhorn)


components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/beacon/processor/CsvExpanderBeaconProcessor.java, line 49 at r2 (raw file):

                }
            }
            if (sum != 0) {

We should record 0 in case data exist.


components/inspectit-ocelot-eum-server/src/test/java/rocks/inspectit/oce/eum/server/beacon/processor/CsvExpanderBeaconProcessorTest.java, line 23 at r1 (raw file):

Previously, Patrick-Eichhorn wrote…

Done.

In my oppinion, we should record 0 as well.

As before, abc should not produce any data, but 0,0 should result in bmr.0=0, bmr.1=0, bmr.sum=0

Copy link
Contributor Author

@Patrick-Eichhorn Patrick-Eichhorn left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @mariusoe)


components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/beacon/processor/CsvExpanderBeaconProcessor.java, line 49 at r2 (raw file):

Previously, mariusoe (Marius Oehler) wrote…

We should record 0 in case data exist.

Done.

mariusoe
mariusoe previously approved these changes Jan 20, 2021
Copy link
Member

@mariusoe mariusoe left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Patrick-Eichhorn)


components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/beacon/processor/CsvExpanderBeaconProcessor.java, line 45 at r3 (raw file):

                    sum += Integer.parseInt(attributes[i]);
                    beacon = beacon.merge(Collections.singletonMap(resultKey, attributes[i]));
                    beacon = beacon.merge(Collections.singletonMap(ATTRIBUTE_KEY + ".sum", String.valueOf(sum)));

Can you move this out of the loop. Now, the sum is merged into the beacon after each attribute, even there are more attributes left. Basically, this has to be done only once after the loop - actually, like it was before just without the if statement. :-)

Copy link
Member

@mariusoe mariusoe left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Patrick-Eichhorn Patrick-Eichhorn merged commit b1d76d9 into inspectIT:master Jan 20, 2021
@Patrick-Eichhorn Patrick-Eichhorn deleted the eum_beacon_processor branch January 20, 2021 10:58
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area/eum-server enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolve 'rt.bmr' data send by Boomerang plugin RT
2 participants