Add cloud logging service feature by Yavor16 · Pull Request #1847 · cloudfoundry/multiapps-controller

Yavor16 · 2026-05-28T06:51:24Z

No description provided.

# Conflicts: # multiapps-controller-process/src/main/resources/org/cloudfoundry/multiapps/controller/process/backup-existing-app.bpmn # multiapps-controller-process/src/main/resources/org/cloudfoundry/multiapps/controller/process/deploy-app.bpmn # multiapps-controller-process/src/main/resources/org/cloudfoundry/multiapps/controller/process/stop-dependent-modules.bpmn # multiapps-controller-process/src/main/resources/org/cloudfoundry/multiapps/controller/process/undeploy-app.bpmn

IvanBorislavovDimitrov · 2026-06-04T11:37:50Z

+import org.cloudfoundry.multiapps.controller.core.auditlogging.model.ConfigurationChangeActions;
+import org.cloudfoundry.multiapps.controller.persistence.model.LoggingConfiguration;
+
+public class CloudLoggingServiceConfigurationAuditLog {


Because SAP Cloud logging service is an internal proprietary service it should not be exposed in the open source. The whole logic related to the service must be move in the internal project.

Move every SAP related thing to the internal project and only leave the what is actually open source in this PR

At least audit logging part maybe can be moved in internal project? SAP Cloud Logging is internal service but API endpoints are public, right?

I don't see any changes here.

IvanBorislavovDimitrov · 2026-06-04T13:28:38Z

+            InputStream serverCaStream = getCredentialInputStream(loggingConfiguration.getServerCa());
+            InputStream clientCertStream = getCredentialInputStream(loggingConfiguration.getClientCert());
+            InputStream clientKeyStream = getCredentialInputStream(loggingConfiguration.getClientKey());


are these streams closed?

IvanBorislavovDimitrov

Additional comments on OperationLogsExporter not yet covered in the earlier review.

sonarqubecloud · 2026-06-11T12:17:13Z

Quality Gate failed

Failed conditions
1 Security Hotspot
77.4% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

theghost5800 · 2026-06-12T10:40:12Z

+import org.cloudfoundry.multiapps.controller.core.auditlogging.model.ConfigurationChangeActions;
+import org.cloudfoundry.multiapps.controller.persistence.model.LoggingConfiguration;
+
+public class CloudLoggingServiceConfigurationAuditLog {


At least audit logging part maybe can be moved in internal project? SAP Cloud Logging is internal service but API endpoints are public, right?

theghost5800 · 2026-06-17T12:16:16Z

+            <dependency>
+                <groupId>io.projectreactor.netty</groupId>
+                <artifactId>reactor-netty-http</artifactId>
+                <version>${reactor-netty.version}</version>
+            </dependency>
+            <dependency>
+                <groupId>io.projectreactor.netty</groupId>
+                <artifactId>reactor-netty-core</artifactId>
+                <version>${reactor-netty.version}</version>
+            </dependency>


Why these dependencies are required? WebClient provided by spring webflux isn't it enough?

I use HttpClient in CloudLoggingServiceHttpClient which required these dependencies

IvanBorislavovDimitrov · 2026-07-06T08:36:27Z

        return executor;
    }

+    @Bean("cloudLoggingServiceAsyncExecutor")


Where is this cloudLoggingServiceAsyncExecutor used?

IvanBorislavovDimitrov · 2026-07-06T08:37:41Z

+        auditLog = new CloudLoggingServiceConfigurationAuditLog(auditLoggingFacade);
+    }
+
+    // --- logCreateLoggingConfiguration ---


remove this?

IvanBorislavovDimitrov · 2026-07-06T08:38:19Z

+        assertEquals(NAMESPACE, identifiers.get("namespace"));
+    }
+
+    // --- logDeleteLoggingConfiguration ---


remove these kinds of comments

IvanBorislavovDimitrov · 2026-07-06T09:39:26Z

+@NamedQuery(name = LoggingConfigurationDto.NamedQueries.FIND_BY_MTA_SPACE_ID_NAMESPACE, query = "SELECT c FROM LoggingConfigurationDto c WHERE c.mtaSpace = :mtaSpace AND c.mtaId = :mtaId AND c.namespace = :namespace")
+@NamedQuery(name = LoggingConfigurationDto.NamedQueries.FIND_BY_MTA_SPACE_ID_NULL_NAMESPACE, query = "SELECT c FROM LoggingConfigurationDto c WHERE c.mtaSpace = :mtaSpace AND c.mtaId = :mtaId AND c.namespace IS NULL")
+@NamedQuery(name = LoggingConfigurationDto.NamedQueries.FIND_ALL_BY_MTA_SPACE_ID, query = "SELECT c FROM LoggingConfigurationDto c WHERE c.mtaSpaceId = :mtaSpaceId")


Can this queries be created by chaining the methods?

IvanBorislavovDimitrov · 2026-07-06T10:03:23Z

+    @Column(name = TableColumnNames.CLOUD_LOGGING_NAMESPACE)
+    private String namespace;
+
+    @Column(name = TableColumnNames.CLOUD_LOGGING_SERVICE_INSTANCE_NAME, nullable = false, length = 50)


Let's remove this length enforcements? Same for others

IvanBorislavovDimitrov · 2026-07-06T11:59:03Z

+    private static final long MAX_LIMIT_REQUEST_SIZE_BYTES = 3 * 1024 * 1024 + 512 * 1024;
+
+    private static final Map<String, WebClient> clientCache = Collections.synchronizedMap(
+        new LinkedHashMap<>(CLIENT_CACHE_MAX_SIZE, 0.75f, true) {


Can we use our impl of CachedMap?

Its in the core package so it is not accessible

IvanBorislavovDimitrov · 2026-07-06T13:14:25Z

+
+    INFO, WARN, DEBUG, ERROR, TRACE;
+
+    private static final Map<LogLevel, List<LogLevel>> logLevelLoggingType = setupLogLevels();


IvanBorislavovDimitrov · 2026-07-06T13:41:31Z

+        sendLogsToCloudLoggingService(externalOperationLogEntryBatches, cloudLoggingServiceWebClient, loggingConfiguration);
+    }
+
+    public List<OperationLogEntry> getUnsendProcessLogs(LoggingConfiguration loggingConfiguration) {


this class does too many things and should be split

still applicable?

IvanBorislavovDimitrov · 2026-07-06T13:43:37Z

+        // NOTE: extractMessage strips one trailing character after .trim() — likely a bug
+        // (the -1 was probably meant to handle a trailing newline that .trim() already removes).
+        // The test pins current behavior; flag for follow-up.


What is this?

IvanBorislavovDimitrov · 2026-07-06T13:45:45Z

+        if (areCloudLoggingParametersInvalid(serviceInstanceName, serviceKeyName)) {
+            return handleMissingServiceKey(isFailSafe, null);
+        }
+        CloudControllerClient client = calculateExternalLoggingServiceConfiguration(destinationOrg, destinationSpace);
+        try {
+            CloudServiceKey loggingServiceKey = client.getServiceKey(serviceInstanceName, serviceKeyName);
+            if (loggingServiceKey != null) {
+                return loggingServiceKey;
+            }


These methods return null or throw exception

IvanBorislavovDimitrov · 2026-07-09T07:52:16Z

+        super(entityManagerFactory);
+    }
+
+    public void storeCloudLoggingServiceConfiguration(LoggingConfiguration loggingConfiguration) {


is this method needed, as it just invokes add(loggingConfiguration)?

IvanBorislavovDimitrov · 2026-07-09T07:58:41Z

+    public LoggingConfiguration getCloudLoggingServiceConfiguration(String mtaSpace, String mtaId, String namespace) {
+        try (EntityManager manager = createEntityManager()) {
+            return findByMtaSpaceIdAndNamespace(manager, mtaSpace, mtaId, namespace).getResultStream()
+                                                                                    .findFirst()
+                                                                                    .map(mapper::fromDto)
+                                                                                    .orElse(null);
+        }
+    }
+
+    public void deleteCloudLoggingServiceConfiguration(String id) {
+        try (EntityManager manager = createEntityManager()) {
+            manager.getTransaction()
+                   .begin();
+            LoggingConfigurationDto dto = manager.find(LoggingConfigurationDto.class, id);
+            if (dto != null) {
+                manager.remove(dto);
+            }
+            manager.getTransaction()
+                   .commit();
+        }


Instead of having so many methods, can we remove these and reuse the pattern from other services like:

public AccessTokenQuery createQuery() { return new AccessTokenQueryImpl(createEntityManager(), accessTokenMapper); } return accessTokenService.createQuery() .userGuid(userGuid) .orderByExpiresAt(OrderDirection.DESCENDING) .list();

IvanBorislavovDimitrov · 2026-07-09T08:01:13Z

+import org.springframework.web.reactive.function.client.WebClient;
+
+@Named("cloudLoggingServiceWebClientCache")
+public class CloudLoggingServiceWebClientCache {


Instead of implementing a new cache. Can you reuse the CachedMap from our code to store the web clients?

IvanBorislavovDimitrov · 2026-07-09T08:13:49Z

+        LoggingConfiguration loggingConfiguration = cloudLoggingServiceConfigurationService.getCloudLoggingServiceConfiguration(
+            context.getVariable(Variables.SPACE_NAME), context.getVariable(Variables.MTA_ID), context.getVariable(Variables.MTA_NAMESPACE));
+
+        if (loggingConfiguration != null) {


as we are trying to get the cloud logging configuration, I guess it is ok to log that it is being fetched. Can we remove the null check here?

IvanBorislavovDimitrov · 2026-07-09T08:22:30Z

+        sendLogsToCloudLoggingService(externalOperationLogEntryBatches, cloudLoggingServiceWebClient, loggingConfiguration);
+    }
+
+    public List<OperationLogEntry> getUnsendProcessLogs(LoggingConfiguration loggingConfiguration) {


still applicable?

IvanBorislavovDimitrov · 2026-07-09T08:23:36Z

+        }
+    }
+
+    public void removeClientFromCache(String operationId) {


what happens with processes that are not in final state? like ERROR?

IvanBorislavovDimitrov · 2026-07-09T08:24:37Z

+    private final CloudLoggingServiceWebClientCache webClientCache;
+
+    public CloudLoggingServiceHttpClient() {
+        this(new CloudLoggingServiceWebClientFactory(), new CloudLoggingServiceWebClientCache());


these classes are beans and still created with new?

This is for the unit tests

IvanBorislavovDimitrov · 2026-07-09T08:25:17Z

+                CloudLoggingServiceUtil.logErrorOrThrowExceptionBasedOnFailSafe(loggingConfiguration, LOGGER,
+                                                                                Messages.FAILED_TO_SEND_LOG_MESSAGE_TO_CLS);
+            }
+        } catch (RuntimeException e) {


still applicable?

IvanBorislavovDimitrov · 2026-07-09T08:45:57Z

+import org.slf4j.LoggerFactory;
+
+@Named("operationLogsExporter")
+public class OperationLogsExporter {


Split this class in multiple classes because it does too many things.

IvanBorislavovDimitrov · 2026-07-09T12:24:36Z

+import org.cloudfoundry.multiapps.controller.core.Messages;
+import org.cloudfoundry.multiapps.controller.core.auditlogging.model.AuditLogConfiguration;
+import org.cloudfoundry.multiapps.controller.core.auditlogging.model.ConfigurationChangeActions;
+import org.cloudfoundry.multiapps.controller.persistence.model.LoggingConfiguration;


I think it will be better if we move this class in the internal project (to be extended in the internal project)

theghost5800 · 2026-07-07T08:08:56Z

+import org.cloudfoundry.multiapps.controller.core.auditlogging.model.ConfigurationChangeActions;
+import org.cloudfoundry.multiapps.controller.persistence.model.LoggingConfiguration;
+
+public class CloudLoggingServiceConfigurationAuditLog {


I don't see any changes here.

theghost5800 · 2026-07-08T14:37:01Z

    private CloudLoggingServiceConfigurationService cloudLoggingServiceConfigurationService;
    @Mock
    private CloudLoggingServiceConfigurationAuditLog cloudLoggingServiceConfigurationAuditLog;
    @InjectMocks


This annotation can be removed now when mocks are injected through the constructor.

theghost5800 · 2026-07-08T14:56:11Z

+        super(entityManagerFactory);
+    }
+
+    public void storeCloudLoggingServiceConfiguration(LoggingConfiguration loggingConfiguration) {


I think we can skip this method and just invoke directly add method.

theghost5800 · 2026-07-09T08:06:27Z

+        }
+    }
+
+    public void updateCloudLoggingServiceConfiguration(LoggingConfiguration loggingConfiguration) {


Why don't use standard update method from base class?

theghost5800 · 2026-07-09T08:09:39Z

+
+    public List<LoggingConfiguration> getAllCloudLoggingServiceConfigurationsFromSpace(String spaceId) {
+        try (EntityManager manager = createEntityManager()) {
+            return manager.createQuery("SELECT c FROM LoggingConfigurationDto c WHERE c.mtaSpaceId = :mtaSpaceId",


Why still is use pure SQL query when you can add criteria buiilder class to filter based on different fields. Take a look at OperationQueryImpl class how it is implemented and used.

theghost5800 · 2026-07-09T13:36:18Z

+                                .trim();
+        if (trimmed.isEmpty()) {
+            return message;
+        } else {


else part can be removed and just return the value without any else block

theghost5800 · 2026-07-09T13:41:38Z

+public class CloudLoggingServiceWebClientCache {
+
+    private static final Logger LOGGER = LoggerFactory.getLogger(CloudLoggingServiceWebClientCache.class);
+    private final Map<String, WebClient> CLIENT_CACHE = Collections.synchronizedMap(new HashMap<>());


Why don't reuse org.cloudfoundry.multiapps.controller.core.model.CachedMap . I have some memories that long time ago we have using Collections.synchronizedMap but there is big performance degradation. ConcurrentHashMap provided by Spring is another recommended map used for concurrency.

theghost5800 · 2026-07-09T13:56:37Z

+
+    @Test
+    void testDeleteConfiguration_nonExistentIdDoesNotThrow() {
+        service.deleteCloudLoggingServiceConfiguration("nonexistent-id");


still missing assertion

theghost5800 · 2026-07-09T13:57:21Z

+        converter = new CloudLoggingServiceMessageConverter();
+    }
+
+    // --- extractLogName ---


Remove odd comments

theghost5800 · 2026-07-09T14:01:47Z

+        flowableFacade.setVariableInParentProcessXSA(execution, variableName, value);
+    }
+
+    protected boolean hasSuperExecution(DelegateExecution execution) {


Check method isRootProcess in the same class. I think it's doing the same thing.

Yavor16 force-pushed the export-logs-to-logging-service-squash branch from 20788df to 7220532 Compare May 28, 2026 06:57

fix rebase problems

bb7fc04

Yavor16 force-pushed the export-logs-to-logging-service-squash branch from c1abde6 to bb7fc04 Compare June 1, 2026 10:51

IvanBorislavovDimitrov reviewed Jun 4, 2026

View reviewed changes

Yavor16 added 2 commits June 10, 2026 10:59

Fix a bug

4a2a63a

Add retry

a42c170

theghost5800 requested changes Jun 17, 2026

View reviewed changes

Fix comments

9558829

Yavor16 force-pushed the export-logs-to-logging-service-squash branch from 70685f6 to 9558829 Compare June 26, 2026 05:36

IvanBorislavovDimitrov reviewed Jul 6, 2026

View reviewed changes

Fix comments from Ivas

7c9bf29

IvanBorislavovDimitrov reviewed Jul 9, 2026

View reviewed changes

theghost5800 reviewed Jul 9, 2026

View reviewed changes


		INFO, WARN, DEBUG, ERROR, TRACE;

		private static final Map<LogLevel, List<LogLevel>> logLevelLoggingType = setupLogLevels();

Uh oh!

Conversation

Yavor16 commented May 28, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IvanBorislavovDimitrov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud Bot commented Jun 11, 2026

Quality Gate failed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment