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:33:35Z

+            return Optional.empty();
+        }
+        String raw = matcher.group(1);
+        return Optional.of(raw.substring(raw.indexOf(".") + 1));


What is the goal of this line? Why split by . ?

we get the name of the file for example if its OPERATION.log we get OPERATION

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-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: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.

+
+    void logDeleteLoggingConfiguration(String username, String spaceId, LoggingConfiguration loggingConfiguration);
+
+    void logGetLoggingConfiguration(String username, String spaceId, LoggingConfiguration loggingConfiguration);


+
+    void logDeleteLoggingConfiguration(String username, String spaceId, LoggingConfiguration loggingConfiguration);
+
+    void logGetLoggingConfiguration(String username, String spaceId, LoggingConfiguration loggingConfiguration);


+
+    void logDeleteLoggingConfiguration(String username, String spaceId, LoggingConfiguration loggingConfiguration);
+
+    void logGetLoggingConfiguration(String username, String spaceId, LoggingConfiguration loggingConfiguration);


+
+    void logUpdateLoggingConfiguration(String username, String spaceId, LoggingConfiguration newConfiguration);
+
+    void logDeleteLoggingConfiguration(String username, String spaceId, LoggingConfiguration loggingConfiguration);


+
+    void logUpdateLoggingConfiguration(String username, String spaceId, LoggingConfiguration newConfiguration);
+
+    void logDeleteLoggingConfiguration(String username, String spaceId, LoggingConfiguration loggingConfiguration);


+
+    void logUpdateLoggingConfiguration(String username, String spaceId, LoggingConfiguration newConfiguration);
+
+    void logDeleteLoggingConfiguration(String username, String spaceId, LoggingConfiguration loggingConfiguration);


+
+    void logCreateLoggingConfiguration(String username, String spaceId, LoggingConfiguration loggingConfiguration);
+
+    void logUpdateLoggingConfiguration(String username, String spaceId, LoggingConfiguration newConfiguration);


+
+    void logCreateLoggingConfiguration(String username, String spaceId, LoggingConfiguration loggingConfiguration);
+
+    void logUpdateLoggingConfiguration(String username, String spaceId, LoggingConfiguration newConfiguration);


+
+    void logCreateLoggingConfiguration(String username, String spaceId, LoggingConfiguration loggingConfiguration);
+
+    void logUpdateLoggingConfiguration(String username, String spaceId, LoggingConfiguration newConfiguration);


+
+public interface CloudLoggingServiceConfigurationAuditLog {
+
+    void logCreateLoggingConfiguration(String username, String spaceId, LoggingConfiguration loggingConfiguration);


IvanBorislavovDimitrov · 2026-07-20T10:35:30Z

+    public LoggingConfiguration getLoggingConfiguration(String mtaSpace, String mtaId, String namespace) {
+        return createQuery().mtaSpace(mtaSpace)
+                            .mtaId(mtaId)
+                            .namespace(namespace)
+                            .list()
+                            .stream()
+                            .findFirst()
+                            .orElse(null);
+    }


Can we just use, createQuery outside of this class?

IvanBorislavovDimitrov · 2026-07-20T10:37:32Z

+import static org.springframework.http.HttpHeaders.CONTENT_TYPE;
+import static org.springframework.http.MediaType.APPLICATION_JSON_VALUE;
+
+@Named("cloudLoggingServiceHttpClient")


This class is a HttpClient and some methods expect passing a webClient. Consider more encapsulation.

Create some facade and rename it.

IvanBorislavovDimitrov · 2026-07-20T10:37:52Z

+        return webClientFactory.createWebClientWithMtls(loggingConfiguration);
+    }
+
+    private ResponseEntity<Void> executeSendLogHttpRequest(WebClient webClient, List<ExternalOperationLogEntry> logEntryBatch) {


Can this webClient be part of the class itself?

IvanBorislavovDimitrov · 2026-07-20T10:39:29Z

+    private CloudServiceKey getCloudLoggingServiceKey(String serviceInstanceName, String serviceKeyName, String destinationOrg,
+                                                      String destinationSpace, boolean isFailSafe) {
+        if (areCloudLoggingParametersInvalid(serviceInstanceName, serviceKeyName)) {
+            throwExceptionIfIsNotFailSafe(isFailSafe, getCloudLoggingServiceKeyErrorMessage());
+            return null;
+        }
+        CloudControllerClient client = calculateExternalLoggingServiceConfiguration(destinationOrg, destinationSpace);
+        try {
+            CloudServiceKey loggingServiceKey = client.getServiceKey(serviceInstanceName, serviceKeyName);
+            if (loggingServiceKey != null) {
+                return loggingServiceKey;
+            }
+            throwExceptionIfIsNotFailSafe(isFailSafe, getCloudLoggingServiceKeyErrorMessage());
+            return null;
+        } catch (CloudOperationException e) {
+            throwExceptionIfIsNotFailSafe(isFailSafe, e.getMessage());
+            return null;
+        }
+    }


These methods return null or throw exception

IvanBorislavovDimitrov · 2026-07-20T10:48:03Z

+@Named("cloudLoggingServiceMessageConverter")
+public class CloudLoggingServiceMessageConverter {
+    private static final Logger LOGGER = LoggerFactory.getLogger(CloudLoggingServiceMessageConverter.class);
+    // SAP Cloud Logging ingest endpoint accepts payloads up to ~4 MB; 3.5 MB leaves headroom for JSON envelope and HTTP framing.


what is this comment for?

IvanBorislavovDimitrov · 2026-07-20T11:18:26Z

+    private boolean areParallelListsConsistent(List<String> messages, List<String> logLevels, List<LocalDateTime> logDates) {
+        return messages.size() <= logLevels.size() && messages.size() <= logDates.size();
+    }


when can we hit this inconsistency?

IvanBorislavovDimitrov · 2026-07-20T11:22:21Z

+public class CloudLoggingServiceWebClientCache {
+
+    private static final Logger LOGGER = LoggerFactory.getLogger(CloudLoggingServiceWebClientCache.class);
+    private final CachedMap<String, WebClient> CLIENT_CACHE = new CachedMap<>(Duration.ofHours(1));


Most operations are shorter than 1 hour. We can reduce this to ~10 minutes

IvanBorislavovDimitrov · 2026-07-20T11:24:20Z

+        if (sslContext == null) {
+            return null;
+        }


Why is null being returned? If we cannot create a web client shouldn't we fail?

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

Fix comments

8e157bb

github-code-quality Bot found potential problems Jul 10, 2026

View reviewed changes

Fix unit tests and undeploy bug

f1ffc33

IvanBorislavovDimitrov reviewed Jul 20, 2026

View reviewed changes


		INFO, WARN, DEBUG, ERROR, TRACE;

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


		void logDeleteLoggingConfiguration(String username, String spaceId, LoggingConfiguration loggingConfiguration);

		void logGetLoggingConfiguration(String username, String spaceId, LoggingConfiguration loggingConfiguration);


		void logUpdateLoggingConfiguration(String username, String spaceId, LoggingConfiguration newConfiguration);

		void logDeleteLoggingConfiguration(String username, String spaceId, LoggingConfiguration loggingConfiguration);


		void logCreateLoggingConfiguration(String username, String spaceId, LoggingConfiguration loggingConfiguration);

		void logUpdateLoggingConfiguration(String username, String spaceId, LoggingConfiguration newConfiguration);


		public interface CloudLoggingServiceConfigurationAuditLog {

		void logCreateLoggingConfiguration(String username, String spaceId, LoggingConfiguration loggingConfiguration);

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