feat: Replace deprecated Doppler recentLogs with Log Cache client - #1237
ZPascal wants to merge 19 commits into
Conversation
|
Thank you for the PR! Compilation is currently failing on the implementation missing the new recentLogs method that you added to the client |
Sorry, something went wrong.
|
Thank you @pivotal-david-osullivan & @anthonydahanne for fixing the tests and the reactor part. I am also working on the issue again from the SAP side and I plan to set up a BBL environment to test the change in a CF environment. Let's sync on Slack how we can handle it together and we can discuss the test setup/steps. |
Sorry, something went wrong.
cool! |
Sorry, something went wrong.
|
|
Sorry, something went wrong.
- The DopplerClient does not work with Loggregator >= 107.0.0, and we cannot obtain "recent" logs. Until this is fixed, we cannot cut a release. That is the only blocking test. - See cloudfoundrygh-1181, cloudfoundrygh-1230, cloudfoundrygh-1237
…CF 2.x line (#1265) * Restrict ApplicationsTests#logs to CF 2.x line - The DopplerClient does not work with Loggregator >= 107.0.0, and we cannot obtain "recent" logs. Until this is fixed, we cannot cut a release. That is the only blocking test. - See gh-1181, gh-1230, gh-1237 * Introduce Applications#logs API that will replace the doppler-based implementation Signed-off-by: Daniel Garnier-Moiroux <daniel.garnier-moiroux@broadcom.com> --------- Signed-off-by: Daniel Garnier-Moiroux <daniel.garnier-moiroux@broadcom.com>
* because of the findFirst() on the envelopes, it could be type OUT or ERR, so we don't really care, as long as the payload is the same. Also, we don't care about the precise argument to the recentLogs call
The old "Applications.logs" method is keept for compatibility and when a log stream is needed. Adding new "logsRecent" method to access the logCache. integration-tests "ApplicationTest.logs" and "ApplicationTest.logsRecent" work. Minor changes in DefaultApplicationsTest in other JUnit tests to make them execute in Eclipse.
Hi @anthonydahanne, I've checked the possible options, and the implementation of the log streaming functionality should definitely be handled in a separate PR. It's out of the scope for the recent logs PR. |
Sorry, something went wrong.
Kehrlann
left a comment
There was a problem hiding this comment.
I think we should not introduce a new method in Applications, see comments in the PR directly.
Sorry, something went wrong.
| .map(EnvelopeBatch::getBatch) | ||
| .map(List::stream) | ||
| .flatMapIterable(envelopeStream -> envelopeStream.collect(Collectors.toList())) |
There was a problem hiding this comment.
| .map(EnvelopeBatch::getBatch) | |
| .map(List::stream) | |
| .flatMapIterable(envelopeStream -> envelopeStream.collect(Collectors.toList())) | |
| .flatMapIterable(EnvelopeBatch::getBatch) |
Sorry, something went wrong.
There was a problem hiding this comment.
I'll try it out.
Sorry, something went wrong.
| /** | ||
| * List the applications logs from logCacheClient. | ||
| * If no messages are available, an empty Flux is returned. | ||
| * | ||
| * @param request the application logs request | ||
| * @return the applications logs | ||
| */ | ||
| Flux<Log> logsRecent(ReadRequest request); |
There was a problem hiding this comment.
According to the original plan, we were to replace Flux<LogMessage> logs(LogsRequest request); by Flux<ApplicationLog> logs(ApplicationLogsRequest request);.
Maybe we should not introduce a new method, but update Flux<ApplicationLog> logs(ApplicationLogsRequest request); to use logcache. Thoughts?
Sorry, something went wrong.
There was a problem hiding this comment.
This would result in an incompatible change. I believe we should not release any incompatible changes in version 5.x.
Sorry, something went wrong.
There was a problem hiding this comment.
Just to clarify, my point is:
- Don't introduce a new method
- Change the implementation of Flux<ApplicationLog> logs(ApplicationLogsRequest request);
This method was introduced specifically for this migration, see this commit: 1664cbf
It did not exist before 5.13.0 (release notes), where we said:
Deprecate doppler-based Applications#logs, prepare API types for the new Loggregator-based client
The idea was to make a method that uses Doppler, then change it to use Loggregator. That's why we introduced Doppler-agnostic types. The whole idea was to do a non-breaking change when swapping out Doppler ; the breaking change would be to remove Flux<LogMessage> logs(LogsRequest request); in v6.
(Also, in practice, this new method Flux<ApplicationLog> logs(ApplicationLogsRequest request); probably never worked for our users, because it's extremely likely they were already using a newer CF deployment version without Doppler when we released it)
Sorry, something went wrong.
There was a problem hiding this comment.
I understand your point, but it does feel quite opaque if we're making under-the-hood calls to another endpoint. I think this is something we should clearly document and communicate to the community so they're aware of the change.
Sorry, something went wrong.
| Mono<String> applicationGuid = | ||
| getAppGuidFromAppName(cloudFoundryOperations, applicationName); |
There was a problem hiding this comment.
If we use Flux<ApplicationLog> logs(ApplicationLogsRequest request); for logs as mentioned further up in the comments, we can have a request that only cares about the application name, not its ID. Makes this test nicer.
Sorry, something went wrong.
There was a problem hiding this comment.
We shouldn’t release a breaking change right away. This change should be released in V6.
Sorry, something went wrong.
|
I'll close the PR. The change has already been handled in #1348. |
Sorry, something went wrong.
Feat: Replace deprecated Doppler recentLogs with Log Cache client
Problem
The DopplerClient.recentLogs() endpoint relies on the Loggregator Traffic Controller /apps/{id}/recentlogs HTTP endpoint, which was removed in Loggregator ≥ 107.0 (shipped with CF Deployment ≥ 24.3 and Tanzu Application Service ≥ 4.0). Calls to this endpoint on modern CF foundations fail at runtime, and the related types (LogsRequest, LogMessage from org.cloudfoundry.doppler) caused compilation errors in the integration tests.
Out of the scope
Fixes #1181 & #1230
Changes
cloudfoundry-client
cloudfoundry-client-reactor
cloudfoundry-operations
Integration & unit tests
Documentation (README.md)
Migration guide
Checklist
Integrationtest results
Logs of the Integration test runThe following command was used for the integration test execution:
Integration Tests
Log-Cache Integration Tests: