Closes #637 - Exposing count of reported resource timing elements #651

ivansenic · 2020-03-30T12:41:35Z

Few hits for the review:

I think without reading a full docs from the Boomerang, it's impossible to review this, especially the parsing of the compressed string for resource timing
As discussed with @JonasKunz I added a BeaconRecorder interfaces that acts on fully processed beacon, extended the manager for this purpose
Currently I am only counting resources.. However, I would advise that we immediately report the load times instead. We will have the COUNT and SUM views, so we would have both the count on how many, plus the loading times.. Imo there is no reason to start only with counting.. Plus, let's agree on the metric name not sure if resource_timing_total is good one. Note that now I could not test the timing values, as these are not reported, thus even the tests would be more complete.
I added the crossOrigin tag as well, think this is important. I report cached tag only if it's same origin, otherwise we can not know.
There are few TODOs to be discussed
Documentation comes on top when we agree on the final metrics

I am here for questions. Would be great if somebody can additionally test this on a meaningful HTMl page.

This change is

JonasKunz

Looks fine to me. Adding the timings should be done using a separate PR imo to keep this one small.

I tested it with the petclinic, where some stuff is chached and some is not, there are even some crossOrigi nrequests. Looked all good.

Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @ivansenic)

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/beacon/recorder/BeaconRecorder.java, line 16 at r1 (raw file):

     * @param beacon Fully-processed beacon
     */
    void record(Beacon beacon);

Maybe explicitly state in the comment that this is invoked with the global tags already configured.

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/beacon/recorder/ResourceTimingBeaconRecorder.java, line 55 at r1 (raw file):

        tags.put("crossOrigin", true);

        RESOURCE_TIMING_TOTAL = MetricDefinitionSettings.builder()

How about simply naming it RESOURCE_TIME ?

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/beacon/recorder/ResourceTimingBeaconRecorder.java, line 97 at r1 (raw file):

        String url = beacon.get("u");

        Optional.ofNullable(beacon.get("restiming"))

I personally find the "traditional" way here more readable:

String resourceTimings = beacon.get("restiming");
if(resourceTimings != null) {
  resourceTimingResults(resourceTimings).forEach(rs -> this.record(rs,url));
}`

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/beacon/recorder/ResourceTimingBeaconRecorder.java, line 133 at r1 (raw file):

     * @return stream
     */
    private Stream<ResourceTimingEntry> resourceTimingResults(String resourceTiming) {

Rename this Method decodeResourceTimings, your name is missing a verb.

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/beacon/recorder/ResourceTimingBeaconRecorder.java, line 154 at r1 (raw file):

     * @return Map where keys are resource URLs and values are the Boomerang compressed resource timing string.
     */
    private Map<String, String> findAllTimingValuesAsText(JsonNode node) {

Another reanming suggestions: How about flattenUrlTrie?

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/beacon/recorder/ResourceTimingBeaconRecorder.java, line 166 at r1 (raw file):

            }
        } else {
            node.fields().forEachRemaining(entry -> this.findAllTimingValuesAsText(entry.getValue(), foundSoFar, prefix + entry.getKey()));

You need to handle the special case of a | key here, which happens if a resource URL is also the prefix of another resource. (See the boomerang documentation)

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/beacon/recorder/ResourceTimingBeaconRecorder.java, line 264 at r1 (raw file):

         */
        public static Optional<ResourceTimingEntry> from(String url, String value) {
            // check if this string contains additional data

How about adding a try / catch for any kind of exception around the method body?
We essentially have no control over value so someone could crash the server by sending malicious data.
An example that comes into my mind here is a NumberFormatException.

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/beacon/recorder/ResourceTimingBeaconRecorder.java, line 285 at r1 (raw file):

            // if empty then it's zero
            Integer[] timings = Arrays.stream(timingsAsBase36Strings)
                    .map(v -> StringUtils.isEmpty(v) ? 0 : Integer.parseInt(v, 36))

Shouldn't an empty string be null instead of 0?

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/configuration/model/ResourceTimingSettings.java, line 16 at r1 (raw file):

     * If resource timing is enabled or not.
     */
    private boolean enabled = true;

No need to set a default value, as the default is specified in application.yml

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/metrics/BeaconMetricManager.java, line 36 at r1 (raw file):

    @Autowired(required = false)
    private List<BeaconRecorder> beaconRecorders;

Did you forget to add the BeaconRecorder for User metrics?

Edit: Nevermind, I saw that you didn't implement this as a recorder. Fine for me.

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/metrics/BeaconMetricManager.java, line 65 at r1 (raw file):

        // allow each beacon recorder to record stuff
        if (!CollectionUtils.isEmpty(beaconRecorders)) {

No real need for the if check here, getTagContextForBeacon shouldn't be that expensive.

components/inspectit-ocelot-eum-server/src/test/java/rocks/inspectit/oce/eum/server/beacon/recorder/ResourceTimingBeaconRecorderTest.java, line 73 at r1 (raw file):

                    "  \"http\": {\n" +
                    "    \"://myhost/\": {\n" +
                    "      \"boomerang/plugins/\": {\n" +

extend this with the speical case of a pipe key

…ents

ivansenic · 2020-04-06T09:58:22Z

@mariusoe I pushed the updates..

However, there are two TODOs left that Jonas did not respond to.. In addition, I have currently defined the metric as the resource_time, but as type long and I am adding 1.0 as time.. This all seems not correct imo.. Let's shortly talk after you check the TODOs maybe.. I also added documentation, so you could have a look on that as well..

ivansenic

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

components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/beacon/recorder/BeaconRecorder.java, line 16 at r1 (raw file):