Closes #1565: [Bug] - No default sample-mode for auto tracing settings

[heiko-holz]

Closes #1565

---

This change is 

[quandor]

Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @heiko-holz)

---

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/config/MethodHookConfigurationResolver.java line 185 at r1 (raw file):

                    .anyMatch(instrumentationRule -> RuleTracingSettings.DEFAULT_SAMPLE_MODE != instrumentationRule.getTracing()
                            .getSampleMode()) ? getAndDetectConflicts(matchedRules, r -> r.getTracing()
                    .getSampleMode(), n -> RuleTracingSettings.DEFAULT_SAMPLE_MODE != n, "the trace sample mode") : RuleTracingSettings.DEFAULT_SAMPLE_MODE);

That code is pretty dense and hard to understand IMHO. Especially the ternary operator doesn't make it easier to read.

I propose the following alternative. We overload getAndDetectConflicts to return a default value if there is no element satisfying the filter. The original method simply forwards to the new one with null as default value to return.

You can see below what I have in mind.

Code snippet:

    builder.sampleMode(getAndDetectConflicts(matchedRules, r -> r.getTracing.getSampleMode(), n -> RuleTracingSettings.DEFAULT_SAMPLE_MODE != n, "the trace sample mode",  RuleTracingSettings.DEFAULT_SAMPLE_MODE);
    ...
    /**
     * Utility function for merging configurations from multiple rules and detecting conflicts.
     * <p>
     *     The same as calling getAndDetectConflicts (rules, getter, filter, exceptionMessage, null);
     * </p>
     */
    private <T> T getAndDetectConflicts(Collection<InstrumentationRule> rules, Function<InstrumentationRule, T> getter, Predicate<? super T> filter, String exceptionMessage) throws ConflictingDefinitionsException {
        return getAndDetectConflicts(rules, getter, filter, exceptionMessage, null);
    }

    /**
     * Utility function for merging configurations from multiple rules and detecting conflicts.
     * This method first calls the given getter on all specified rules and filters the results using the given filter.
     * <p>
     * It then ensures that all provided values are equal, otherwise raises an exception with the given message
     *
     * @param rules            the rules on which the getter will be called
     * @param getter           the getter function to call on each rule
     * @param filter           the predicate to filter the results of the getters with, e.g. Objects#nonNull
     * @param exceptionMessage the name of the setting to print in an exception message
     * @param defaultValue     the value which is returned, if filter does not match for any element of rules
     * @param <T>              the type of the value which is being queried
     *
     * @return null if none of the rules have a setting matching the given filter. Otherwise returns the setting found.
     *
     * @throws ConflictingDefinitionsException thrown if a conflicting setting is detected
     */
    private <T> T getAndDetectConflicts(Collection<InstrumentationRule> rules, Function<InstrumentationRule, T> getter, Predicate<? super T> filter, String exceptionMessage, T defaultValue) throws ConflictingDefinitionsException {
        Optional<InstrumentationRule> firstMatch = rules.stream().filter(r -> filter.test(getter.apply(r))).findFirst();
        if (firstMatch.isPresent()) {
            T value = getter.apply(firstMatch.get());
            Optional<InstrumentationRule> secondMatch = rules.stream()
                    .filter(r -> r != firstMatch.get())
                    .filter(r -> filter.test(getter.apply(r)))
                    .filter(r -> !Objects.equals(getter.apply(r), value))
                    .findFirst();
            if (secondMatch.isPresent()) {
                throw new ConflictingDefinitionsException(firstMatch.get(), secondMatch.get(), exceptionMessage);
            } else {
                return value;
            }
        } else {
            return defaultValue;
        }
    }

[heiko-holz]

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/config/MethodHookConfigurationResolver.java line 185 at r1 (raw file):

Previously, quandor (Jochen Just) wrote…

That code is pretty dense and hard to understand IMHO. Especially the ternary operator doesn't make it easier to read.

I propose the following alternative. We overload getAndDetectConflicts to return a default value if there is no element satisfying the filter. The original method simply forwards to the new one with null as default value to return.

You can see below what I have in mind.

Thanks for your suggestion, I agree!
I have changed the code accordingly.

[codecov]

Codecov Report

Merging #1566 (6d5c28d) into master (3152c9a) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1566      +/-   ##
============================================
- Coverage     78.28%   78.22%   -0.06%     
+ Complexity     2404     2403       -1     
============================================
  Files           246      246              
  Lines          7909     7911       +2     
  Branches        936      936              
============================================
- Hits           6191     6188       -3     
- Misses         1303     1308       +5     
  Partials        415      415              

Impacted Files	Coverage Δ
...del/instrumentation/rules/RuleTracingSettings.java	100.00% <100.00%> (ø)
...tation/config/MethodHookConfigurationResolver.java	96.18% <100.00%> (+0.03%)	⬆️
...in/java/rocks/inspectit/ocelot/core/AgentImpl.java	61.82% <0.00%> (-7.27%)	⬇️
...nspectit/ocelot/core/utils/HighPrecisionTimer.java	89.06% <0.00%> (-1.56%)	⬇️

[quandor]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @heiko-holz)