feat(system-exclude-span-rules): system exclude span rules

[SrikarMannepalli]

Description

This PR adds support for defining system level exclude span rules and overriding(enabling/disabling) them at a tenant level.

Testing

Added new unit tests for all new code, and ran all the existing unit tests.

Checklist:

• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• Any dependent changes have been merged and published in downstream modules

[codecov]

Codecov Report

Merging #133 (f6e9eaf) into main (10eec87) will increase coverage by 0.38%.
The diff coverage is 88.70%.

❗ Current head f6e9eaf differs from pull request most recent head a92dd92. Consider uploading reports for the commit a92dd92 to get more accurate results

@@             Coverage Diff              @@
##               main     #133      +/-   ##
============================================
+ Coverage     84.67%   85.06%   +0.38%     
- Complexity      437      453      +16     
============================================
  Files            47       47              
  Lines          2160     2216      +56     
  Branches         82       90       +8     
============================================
+ Hits           1829     1885      +56     
+ Misses          276      273       -3     
- Partials         55       58       +3     

Flag	Coverage Δ
integration	85.06% <88.70%> (+0.38%)	⬆️
unit	84.18% <88.70%> (+0.41%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ig/service/SpanProcessingConfigServiceFactory.java	0.00% <0.00%> (ø)
...onfig/service/SpanProcessingConfigServiceImpl.java	85.52% <91.22%> (+5.16%)	⬆️
...fig/service/SpanProcessingConfigServiceModule.java	72.72% <100.00%> (+6.06%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[skjindal93]

Can we add validation for not allowing updates for system level rules, apart from the disabled flag?

We should also add validation in delete. Deletion of system rules shouldn't be allowed. We can have this check within the delete API impl

[skjindal93]

Can we move the configs to a separate config class? Or, may be to a separate method. Would prefer a separate config class, if it isn't much refactor

We can also directly define the default exclusion rules config as span.processing.config.service.system.exclude.span.rules, to avoid multiple if block nesting

[SrikarMannepalli]

I have added separate methods for reading configs and conversion for now.
I am thinking of taking up a total refactor(config class, separate manager for exclude rules) separately. What do you suggest?

[skjindal93]

This looks complex. I don't think there was a need to filter out overrides from persisted exclude span rules. We could have actually just sent out the entire persisted exclude span rules + non overriden system exclude rules

Something like this

Set<String> userExcludeSpanRuleIds = excludeSpanRules.stream()
    .map(excludeSpanRule -> excludeSpanRule.getRule().getId()).collect(
        Collectors.toUnmodifiableSet());

// all the system level configs, replaced by the overridden configs in case of overrides
List<ExcludeSpanRule> nonOverridenExcludeSpanRules =
    systemLevelExcludeSpanRules.stream()
        .filter(
            systemExcludeRule -> !userExcludeSpanRuleIds.contains(systemExcludeRule.getId()))
        .collect(Collectors.toUnmodifiableList());

And, we can then convert the nonOverridenExcludeSpanRules to List<ExcludeSpanRuleDetails> while sending it in the response

[SrikarMannepalli]

I had taken this route so as to ensure that the system exclude rules(overridden or not) would end up in the bottom and the UI wouldn't have to worry about the ordering.

[skjindal93]

How are we taking care of the fact, if a customer disables the system exclude rule, we need to store is as disabled?

[SrikarMannepalli]

It is handled in building updated rule part on L189. The new rule would be built from the existing system exclude rule and the update exclude rule, and upserted.

[skjindal93]

If we convert list into a map, then we can directly use Maps.difference(userExcludeSpanRuleIdToRuleMap, systemExcludeSpanRuleIdToRuleMap). You can use

entriesInCommon for overriden rules
entriesOnLeft for only user rules
entriesOnRight for only system rules

[SrikarMannepalli]

But we would not be able to retain order among rules within the same groups right?🤔 Am I missing something?

[github-actions]

Unit Test Results

  25 files  ±0    25 suites  ±0   46s ⏱️ -2s
105 tests ±0  105 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit f786710. ± Comparison against base commit 10eec87.