You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@felix.apache.org by GitBox <gi...@apache.org> on 2021/11/30 16:28:04 UTC

[GitHub] [felix-dev] ghenzler commented on a change in pull request #121: FELIX-6473 - Make BundlesStartedCheck simpler

ghenzler commented on a change in pull request #121:
URL: https://github.com/apache/felix-dev/pull/121#discussion_r759451890



##########
File path: healthcheck/generalchecks/src/main/java/org/apache/felix/hc/generalchecks/BundlesStartedCheck.java
##########
@@ -79,78 +82,85 @@ protected void activate(BundleContext bundleContext, Config config) {
         this.includesRegex = Pattern.compile(config.includesRegex());
         this.excludesRegex = StringUtils.isNotBlank(config.excludesRegex()) ? Optional.of(Pattern.compile(config.excludesRegex())) : Optional.empty();
         this.useCriticalForInactive = config.useCriticalForInactive();
-        LOG.debug("Activated bundles started HC for includesRegex={} excludesRegex={}% useCriticalForInactive={}", includesRegex, excludesRegex, useCriticalForInactive);
+        LOG.info("Activated bundles started HC for includesRegex={} excludesRegex={}% useCriticalForInactive={}", includesRegex, excludesRegex, useCriticalForInactive);
     }
 
-
     @Override
     public Result execute() {
         FormattingResultLog log = new FormattingResultLog();
+        List<Bundle> bundles = Arrays.asList(bundleContext.getBundles());
+        AtomicLong checkedBundles = new AtomicLong();
+        long activeBundles = bundles.stream()
+                .filter(this::isIncluded)
+                .filter(this::isNotExcluded)
+                .filter(this::isNotFragment)
+                .filter(this::isNotLazyStarting)
+                .peek(bundle -> checkedBundles.incrementAndGet())
+                .filter(bundle -> isActive(log, bundle))
+                .count();
+        log.info("Bundles total: {}, expected to be active: {}, active: {}.", bundles.size(), checkedBundles, activeBundles);
+        return new Result(log);
+    }
 
-        Bundle[] bundles = this.bundleContext.getBundles();
-        log.debug("Framwork has {} bundles in total", bundles.length);
-
-        int countExcluded = 0;
-        int relevantBundlesCount = 0;
-        int inactiveCount = 0;
-        for (Bundle bundle : bundles) {
-            String bundleSymbolicName = bundle.getSymbolicName();
-            int bundleState = bundle.getState();
-
-            if (!includesRegex.matcher(bundleSymbolicName).matches()) {
-                LOG.debug("Bundle {} not matched by {}", bundleSymbolicName, includesRegex);
-                continue;
-            }
-
-            if (excludesRegex.isPresent() && excludesRegex.get().matcher(bundleSymbolicName).matches()) {
-                LOG.debug("Bundle {} excluded {}", bundleSymbolicName, excludesRegex);
-                countExcluded ++;
-                continue;
-            }
-            relevantBundlesCount++;
-
-            boolean bundleIsLogged = false;
-            if (bundleState != Bundle.ACTIVE) {
-                // support lazy activation (https://www.osgi.org/developer/design/lazy-start/)
-                if (bundleState == Bundle.STARTING && isLazyActivation(bundle)) {
-                    LOG.debug("Ignoring lazily activated bundle {}", bundleSymbolicName);
-                } else  if (StringUtils.isNotBlank(bundle.getHeaders().get(Constants.FRAGMENT_HOST))) {
-                    LOG.debug("Ignoring bundle fragment: {}", bundleSymbolicName);
-                } else {
-                    String msg = "Inactive bundle {} {}: {}";
-                    Object[] msgObjs = new Object[] {bundle.getBundleId(), bundleSymbolicName, getStateLabel(bundleState)};
-                    LOG.debug(msg, msgObjs);
-                    if(useCriticalForInactive) {
-                        log.critical(msg, msgObjs);
-                    } else {
-                        log.warn(msg, msgObjs);
-                    }
-                    bundleIsLogged = true;
-                    inactiveCount++;
-                }
-            }
-            if(!bundleIsLogged) {
-                log.debug("Bundle {} {}: {}", bundle.getBundleId(), bundleSymbolicName, getStateLabel(bundleState));
-            }
+    private boolean isNotFragment(Bundle bundle) {
+        boolean isFragment = StringUtils.isNotBlank(bundle.getHeaders().get(Constants.FRAGMENT_HOST));
+        if (isFragment) {
+            LOG.debug("Ignoring bundle fragment: {}", bundle.getSymbolicName());
         }
-
-        String includeMsg = !includesRegex.pattern().equals(".*") ? " for pattern " + includesRegex.pattern(): "";
-        String baseMsg = relevantBundlesCount + " bundles" + includeMsg;
-        String excludedMsg = countExcluded > 0 ? " (" + countExcluded + " excluded via pattern " + excludesRegex.get().pattern() + ")" : "";

Review comment:
       Showing the exclude pattern in the result for the case it kicks in is quite useful IMHO - with this PR I only see `Bundles total: 578, expected to be active: 567, active: 566` and I don't immediately see the reason why 567 out of 578 are expected (someone might think this is due to the number 567 being configured when in reality it is an exclude regex)

##########
File path: healthcheck/generalchecks/src/main/java/org/apache/felix/hc/generalchecks/BundlesStartedCheck.java
##########
@@ -79,78 +82,85 @@ protected void activate(BundleContext bundleContext, Config config) {
         this.includesRegex = Pattern.compile(config.includesRegex());
         this.excludesRegex = StringUtils.isNotBlank(config.excludesRegex()) ? Optional.of(Pattern.compile(config.excludesRegex())) : Optional.empty();
         this.useCriticalForInactive = config.useCriticalForInactive();
-        LOG.debug("Activated bundles started HC for includesRegex={} excludesRegex={}% useCriticalForInactive={}", includesRegex, excludesRegex, useCriticalForInactive);
+        LOG.info("Activated bundles started HC for includesRegex={} excludesRegex={}% useCriticalForInactive={}", includesRegex, excludesRegex, useCriticalForInactive);
     }
 
-
     @Override
     public Result execute() {
         FormattingResultLog log = new FormattingResultLog();
+        List<Bundle> bundles = Arrays.asList(bundleContext.getBundles());
+        AtomicLong checkedBundles = new AtomicLong();
+        long activeBundles = bundles.stream()
+                .filter(this::isIncluded)
+                .filter(this::isNotExcluded)
+                .filter(this::isNotFragment)
+                .filter(this::isNotLazyStarting)
+                .peek(bundle -> checkedBundles.incrementAndGet())
+                .filter(bundle -> isActive(log, bundle))
+                .count();
+        log.info("Bundles total: {}, expected to be active: {}, active: {}.", bundles.size(), checkedBundles, activeBundles);
+        return new Result(log);
+    }
 
-        Bundle[] bundles = this.bundleContext.getBundles();
-        log.debug("Framwork has {} bundles in total", bundles.length);
-
-        int countExcluded = 0;
-        int relevantBundlesCount = 0;
-        int inactiveCount = 0;
-        for (Bundle bundle : bundles) {
-            String bundleSymbolicName = bundle.getSymbolicName();
-            int bundleState = bundle.getState();
-
-            if (!includesRegex.matcher(bundleSymbolicName).matches()) {
-                LOG.debug("Bundle {} not matched by {}", bundleSymbolicName, includesRegex);
-                continue;
-            }
-
-            if (excludesRegex.isPresent() && excludesRegex.get().matcher(bundleSymbolicName).matches()) {
-                LOG.debug("Bundle {} excluded {}", bundleSymbolicName, excludesRegex);
-                countExcluded ++;
-                continue;
-            }
-            relevantBundlesCount++;
-
-            boolean bundleIsLogged = false;
-            if (bundleState != Bundle.ACTIVE) {
-                // support lazy activation (https://www.osgi.org/developer/design/lazy-start/)
-                if (bundleState == Bundle.STARTING && isLazyActivation(bundle)) {
-                    LOG.debug("Ignoring lazily activated bundle {}", bundleSymbolicName);
-                } else  if (StringUtils.isNotBlank(bundle.getHeaders().get(Constants.FRAGMENT_HOST))) {
-                    LOG.debug("Ignoring bundle fragment: {}", bundleSymbolicName);
-                } else {
-                    String msg = "Inactive bundle {} {}: {}";
-                    Object[] msgObjs = new Object[] {bundle.getBundleId(), bundleSymbolicName, getStateLabel(bundleState)};
-                    LOG.debug(msg, msgObjs);
-                    if(useCriticalForInactive) {
-                        log.critical(msg, msgObjs);
-                    } else {
-                        log.warn(msg, msgObjs);
-                    }
-                    bundleIsLogged = true;
-                    inactiveCount++;
-                }
-            }
-            if(!bundleIsLogged) {
-                log.debug("Bundle {} {}: {}", bundle.getBundleId(), bundleSymbolicName, getStateLabel(bundleState));
-            }
+    private boolean isNotFragment(Bundle bundle) {
+        boolean isFragment = StringUtils.isNotBlank(bundle.getHeaders().get(Constants.FRAGMENT_HOST));
+        if (isFragment) {
+            LOG.debug("Ignoring bundle fragment: {}", bundle.getSymbolicName());
         }
-
-        String includeMsg = !includesRegex.pattern().equals(".*") ? " for pattern " + includesRegex.pattern(): "";
-        String baseMsg = relevantBundlesCount + " bundles" + includeMsg;
-        String excludedMsg = countExcluded > 0 ? " (" + countExcluded + " excluded via pattern " + excludesRegex.get().pattern() + ")" : "";
-        if (inactiveCount > 0) {
-            log.info("Found  " + inactiveCount + " inactive of " + baseMsg + excludedMsg);
-        } else {
-            log.info("All " + baseMsg + " are started" + excludedMsg);
+        return !isFragment;
+    }
+    
+    private boolean isNotLazyStarting(Bundle bundle) {
+        // support lazy activation (https://www.osgi.org/developer/design/lazy-start/)
+        boolean isLazyStarting = bundle.getState() == Bundle.STARTING && isLazyActivation(bundle);
+        if (isLazyStarting) {
+            LOG.debug("Ignoring lazily activated bundle {}", bundle.getSymbolicName());
         }
+        return !isLazyStarting;
+    }
 
-        return new Result(log);
+    private boolean isIncluded(Bundle bundle) {
+        boolean matches = includesRegex.matcher(bundle.getSymbolicName()).matches();
+        if (matches) {
+            LOG.debug("Bundle {} not matched by {}", bundle.getSymbolicName(), includesRegex);
+        }
+        return matches;
+    }
+    
+    private boolean isNotExcluded(Bundle bundle) {
+        boolean matches = excludesRegex.isPresent() ? excludesRegex.get().matcher(bundle.getSymbolicName()).matches() : false;
+        if (matches) {
+            LOG.debug("Bundle {} excluded {}", bundle.getSymbolicName(), excludesRegex);
+        }
+        return !matches;
     }
 
-    private static boolean isLazyActivation(Bundle b) {
+    private boolean isLazyActivation(Bundle b) {
         return Constants.ACTIVATION_LAZY.equals(b.getHeaders().get(Constants.BUNDLE_ACTIVATIONPOLICY));
     }
 
-    private static String getStateLabel(int state) {
+    private boolean isActive(FormattingResultLog log, Bundle bundle) {
+        boolean isActive = bundle.getState() == Bundle.ACTIVE;
+        String bundleInfo = getBundleInfo(bundle);
+        if (isActive) {
+            log.debug(bundleInfo);
+        } else {
+            String msg = "Inactive " + bundleInfo;
+            if (useCriticalForInactive) {
+                log.critical(msg);
+            } else {
+                log.warn(msg);
+            }
+        }
+        return isActive;
+    }
+
+    private String getBundleInfo(Bundle bundle) {
+        return String.format("bundle with id:%d, name:%s, state:%s)", bundle.getBundleId(), 

Review comment:
       Nitpick: the current format is just `Bundle {} {}: {}` (without labelling `id`/`name`/`state` on every line)
   I think the values are self-explanatory in this context, I would keep it as is




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@felix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org