Add logfile actuator #1499

tscrypter · 2025-04-03T02:21:40Z

Description

This PR adds a Logfile actuator. This endpoint will expose the contents of an logfile when it's configured. To log to a file, the application will need to make use of Third Party Logging providers.

The Third-party logging provider is responsible for log rotation.

This actuator can be configured with settings prefixed with Management:Endpoints:Logfile:

Key	Description	Default
Enabled	Whether the endpoint is enabled	true
ID	The unique ID of the endpoint	logfile
Path	The relative path at which the endpoint is exposed	logfile
RequiredPermissions	Permissions required to access the endpoint, when running on Cloud Foundry	Restricted
AllowedVerbs	An array of HTTP verbs the endpoint is exposed at.	GET
FilePath	The path to the log file that will be exposed. If relative, will be relative to the applications working directory.

Fixes #1496

Quality checklist

Your code complies with our Coding Style.
You've updated unit and/or integration tests for your change, where applicable.
You've updated documentation for your change, where applicable.
If your change affects other repositories, such as Documentation, Samples and/or MainSite, add linked PRs here.
There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.
You've added required license files and/or file headers (explaining where the code came from with proper attribution), where code is copied from StackOverflow, a blog, or OSS.

* 🧪 Add tests for endpoint options Add tests ensuring defaults are sell and can be overridden. * ✅ Implement endpoint options * ♻️ Refactor endpoint options and tests Fix warnings in tests as well, add reference in property docs, and update public APIs * 🧪 Add failing test for relative path Add a test that ensures config with relative path is relative to entry assembly * ✅ Add GetLogFilePath method Implement the GetLogFilePath to support relative paths * ♻️ Cleanup GetFilePath removed the hardcoding of the path to make use of the configuration options * 🧪 Test Options ✅ Implement test for Options * ♻️ Add tests to fix GetLogFilePath * 🧪 Add test for missing filepath config * ✅ Handle missing filepath config * ♻️ Make getlogfile more readable * 🧪 Add InvokeAsync test * ✅ Implement InvokeAsync InvokeAsync will return the contents of the log file * 🧪 Add test for InvokeAsync with no filepath When no file path is configured, InvokeAsync should return an empty string * ✅ Handle no file path in InvokeAsync * register with All actuators * Add to ActuatorHostBuilderTest * 🧪 Add middleware test

tscrypter · 2025-04-03T02:28:41Z

I'll update this PR with a link to a documentation update PR. I also wanted to check if the Management samples would be the correct one to update.

TimHess · 2025-04-03T20:24:13Z

Hi @tscrypter,
Thanks for getting this going, its looking like a good start. Some notes:

Continue to ignore any failure with the Steeltoe Sonar Analysis check (if it happens again), we're working on that separately
Can you change it to "Log File" and LogFile instead of "Logfile" or Logfile in code and comments (the path /actuator/logfile is fine though)
Do you have any concerns with using File.ReadAllTextAsync on larger text files? I don't know what the best option will be, but this also doesn't seem particularly compatible with reading a range of the file

tscrypter · 2025-04-04T01:59:14Z

Hi @tscrypter, Thanks for getting this going, its looking like a good start. Some notes:

Continue to ignore any failure with the Steeltoe Sonar Analysis check (if it happens again), we're working on that separately

Can you change it to "Log File" and LogFile instead of "Logfile" or Logfile in code and comments (the path /actuator/logfile is fine though)

Do you have any concerns with using File.ReadAllTextAsync on larger text files? I don't know what the best option will be, but this also doesn't seem particularly compatible with reading a range of the file

Hi @TimHess ,

Of course, I can make those updates tonight.

Good catch on the File.ReadAllTextAsync. I can update this to read bytes while I implement the range header support.

Regarding handling large log file sizes, maybe introducing a MaxBytes config setting. If not set, the endpoint can retrieve the entire file; else, return no more than MaxBytes.

Assuming 2048 byte log file:

MaxBytes	Range Header	Data returned
		2048 bytes
1024		first 1024 bytes
	bytes=0-1024	first 1024 bytes
1024	bytes=1000-2048	bytes 1000-2024

bart-vmware

The Sonar issue has been fixed, please merge/rebase this PR on main. TestContext was renamed to SteeltoeTestContext, please adapt tests accordingly to fix build errors. In tests, async methods that take a CancellationToken (such as app.StartAsync and httpClient.GetAsync) should be passed TestContext.Current.CancellationToken, this is to prepare for xUnit3.

To fix PublicAPI warnings, it's easiest to clear PublicAPI.Unshipped.txt, close the file, and then use the roslyn fixer (project-wide) to recreate the file.

src/Management/src/Endpoint/Actuators/LogFile/ConfigureLogFileEndpointOptions.cs

src/Management/src/Endpoint/Actuators/LogFile/EndpointServiceCollectionExtensions.cs

src/Management/src/Endpoint/Actuators/LogFile/LogFileEndpointHandler.cs

src/Management/src/Endpoint/Actuators/LogFile/LogFileEndpointOptions.cs

src/Management/test/Endpoint.Test/Actuators/LogFile/EndpointMiddlewareTest.cs

src/Management/test/Endpoint.Test/Actuators/LogFile/EndpointServiceCollectionTest.cs

src/Management/test/Endpoint.Test/Actuators/LogFile/LogFileEndpointTest.cs

Co-authored-by: Bart Koelman <104792814+bart-vmware@users.noreply.github.com>

* Replace delimited with separated * Review feedback

…ions for WindowsNetworkFileShare (SteeltoeOSS#1503) * Support network shares with disk health contributor * Embed Win32Exceptions for WindowsNetworkFileShare connection errors rather than translating --------- Co-authored-by: Bart Koelman <104792814+bart-vmware@users.noreply.github.com>

bart-vmware

Thanks for addressing the feedback. I wrongly commented about clearing PublicAPI.Unshipped.txt. The first line (#nullable enable) needs to stay, and then the analyzer can refill it.

Do you need some tips regarding ranges and file I/O, or did you just not get to it yet?

src/Management/test/Endpoint.Test/Actuators/LogFile/EndpointMiddlewareTest.cs

bart-vmware · 2025-04-09T09:09:12Z

src/Management/src/Endpoint/Actuators/LogFile/ConfigureLogFileEndpointOptions.cs

+using Microsoft.Extensions.Configuration;
+using Steeltoe.Management.Endpoint.Configuration;
+
+namespace Steeltoe.Management.Endpoint.Actuators.Logfile;


The namespaces still have the old casing (also in tests).

src/Management/src/Endpoint/Actuators/LogFile/LogFileEndpointHandler.cs

…andler.cs Co-authored-by: Bart Koelman <104792814+bart-vmware@users.noreply.github.com>

…ddlewareTest.cs Co-authored-by: Bart Koelman <104792814+bart-vmware@users.noreply.github.com>

tscrypter · 2025-04-09T13:58:01Z

Thanks for addressing the feedback. I wrongly commented about clearing PublicAPI.Unshipped.txt. The first line (#nullable enable) needs to stay, and then the analyzer can refill it.

Do you need some tips regarding ranges and file I/O, or did you just not get to it yet?

Thanks @bart-vmware for the guidance. I haven't gotten to it yet, I will likely have some time this evening or tomorrow. I would like your input on the potential implementation before I refactor the code to support it.

I plan on introducing LogFileEndpointRequest and LogFileEndpointResponse classes for the additional range parameters. I'll refactor the ILogFileEndpointHandler interface with these and get the tests that are currently in place back to working.

For the LogFileEndpointMiddleware, I know I'll have to override the ParseRequestAsync to read the original HttpRequest headers, but I think I'll also have to override the WriteResponseAsync (to put the correct response headers). I also don't think I can use the base EndpointMiddleware.WriteResponseAsync because the LogFileEndpointResponse shouldn't be JsonSerialized.

bart-vmware · 2025-04-09T15:44:53Z

Thanks for addressing the feedback. I wrongly commented about clearing PublicAPI.Unshipped.txt. The first line (#nullable enable) needs to stay, and then the analyzer can refill it.
Do you need some tips regarding ranges and file I/O, or did you just not get to it yet?

Thanks @bart-vmware for the guidance. I haven't gotten to it yet, I will likely have some time this evening or tomorrow. I would like your input on the potential implementation before I refactor the code to support it.

I plan on introducing LogFileEndpointRequest and LogFileEndpointResponse classes for the additional range parameters. I'll refactor the ILogFileEndpointHandler interface with these and get the tests that are currently in place back to working.

For the LogFileEndpointMiddleware, I know I'll have to override the ParseRequestAsync to read the original HttpRequest headers, but I think I'll also have to override the WriteResponseAsync (to put the correct response headers). I also don't think I can use the base EndpointMiddleware.WriteResponseAsync because the LogFileEndpointResponse shouldn't be JsonSerialized.

Yes, that sounds about right. Some additional thoughts:

Perhaps make the endpoint return IAsyncEnumerable<byte[]> (4-16k buffer) to stream large files without fully loading them into memory first? Good for scalability, but kinda conflicts with the next bullet.
What's a bit odd is that Spring returns Content-Type: text/plain;charset=UTF-8, but with a range it may not be proper UTF-8 at all (thinking about BOM, multi-byte chars and surrogate pairs).
Should return HTTP 416 on multi-range requests, see https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Range
We don't want to lock while reading a file, potentially preventing the logger from writing to it. So should probably open with FileShare.ReadWrite, but I haven't tested any of that.
May want to use HTTP compression if the client supports it, don't know if this is feasible

bart-vmware

Nice progress, I've added a few remarks.

bart-vmware · 2025-04-15T06:30:17Z

src/Management/src/Endpoint/Actuators/LogFile/LogFileEndpointHandler.cs

+            FileInfo logFile = new FileInfo(logFilePath);
+            var logFileResult = new LogFileEndpointResponse(await File.ReadAllTextAsync(logFilePath, cancellationToken),
+                logFile.Length,
+                Encoding.Default, // StreamReader.CurrentEncoding is not reliable based on unit tests, so just use default for this .NET implementation


We can't make assumptions about the file encoding. It may vary per logger provider, over which Steeltoe has no control. For example, in Serilog, it can be overridden.

At the HTTP level, we're using bytes for ranges. The easiest would be to leave out charset in the response Content-Type header. If we want to include it, it should originate from the endpoint configuration that Steeltoe users must set explicitly, depending on their logger framework.

bart-vmware · 2025-04-15T06:36:57Z

src/Management/src/Endpoint/Actuators/LogFile/ConfigureLogFileEndpointOptions.cs

+        options.AllowedVerbs.Add("Get");
+        options.AllowedVerbs.Add("Head");


EndpointOptions provides a virtual method GetDefaultAllowedVerbs to set these. For example usage, see LoggersEndpointOptions.

bart-vmware · 2025-04-15T06:47:31Z

src/Management/src/Endpoint/Actuators/LogFile/LogFileEndpointRequest.cs

+    public long? RangeStartByte { get; } = rangeStartByte;
+    public long? RangeLastByte { get; } = rangeLastByte;
+    public bool ReadContent { get; } = readContent;


As described here, both the start and end positions are optional, so using nullable is good here. But let's make LogFileEndpointRequest in the pipeline signature non-nullable. This is a public API. Otherwise, if we need to pass extra required information in the future, we need to take a breaking change. Adding an internal static get-only None property is fine if you're concerned with reducing allocations.

bart-vmware · 2025-04-15T06:50:58Z

src/Management/src/Endpoint/Actuators/LogFile/LogFileEndpointResponse.cs

+{
+    public string? Content { get; } = content;
+    public Encoding? LogFileEncoding { get; } = logFileEncoding;
+    public long? ContentLength { get; } = contentLength;


Based on this, we may want to add the total file size, so it can be returned in the Content-Range header.

bart-vmware · 2025-04-15T06:56:39Z

src/Management/test/Endpoint.Test/Actuators/LogFile/EndpointMiddlewareTest.cs

+        HttpResponseMessage response = await client.SendAsync(headRequest, TestContext.Current.CancellationToken);
+
+        Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+        Assert.Equal("text/plain", response.Content.Headers.ContentType?.MediaType);


Please avoid using the ?. operator in assertions, because no assertion is performed if the value is null at runtime. To ensure it is non-null, assert on that first.

bart-vmware · 2025-04-17T07:33:25Z

/azp run Steeltoe.All

azure-pipelines · 2025-04-17T07:33:35Z

Azure Pipelines successfully started running 1 pipeline(s).

sonarqubecloud · 2025-04-17T07:57:19Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

bart-vmware · 2025-05-02T14:51:26Z

@tscrypter Are you still working on this?

tscrypter · 2025-05-02T20:52:51Z

@tscrypter Are you still working on this?

Hi @bart-vmware, yes I am. Sorry for the delay, some critical issues came up at work. I'm hoping to have this wrapped up this weekend.

tscrypter added 2 commits April 2, 2025 22:58

fix build warnings

a0e8fe2

fix style in Logfile actuator files

711c98f

tscrypter added 2 commits April 4, 2025 09:33

♻️ Rename Logfile to LogFile actuator

ee6cf15

Merge branch 'main' into main

7f8d4f9

bart-vmware requested changes Apr 7, 2025

View reviewed changes

tscrypter and others added 9 commits April 7, 2025 08:56

suggested doc update for LogFile svc extensions

e48a003

Co-authored-by: Bart Koelman <104792814+bart-vmware@users.noreply.github.com>

Suggested docs update for LogFile options

d361f82

Co-authored-by: Bart Koelman <104792814+bart-vmware@users.noreply.github.com>

Replace delimited with separated (SteeltoeOSS#1500)

26511de

* Replace delimited with separated * Review feedback

Address PR feedback

f1f808b

Address PR feedback

ee63559

🚨 Fix public API entries

88324e5

Merge branch 'main' into main

52cd3b0

Merge branch 'SteeltoeOSS:main' into main

04eb095

bart-vmware reviewed Apr 9, 2025

View reviewed changes

tscrypter and others added 3 commits April 9, 2025 09:13

Update src/Management/src/Endpoint/Actuators/LogFile/LogFileEndpointH…

013e333

…andler.cs Co-authored-by: Bart Koelman <104792814+bart-vmware@users.noreply.github.com>

Update src/Management/test/Endpoint.Test/Actuators/LogFile/EndpointMi…

5367550

…ddlewareTest.cs Co-authored-by: Bart Koelman <104792814+bart-vmware@users.noreply.github.com>

Update src/Management/test/Endpoint.Test/Actuators/LogFile/EndpointMi…

2b016ef

…ddlewareTest.cs Co-authored-by: Bart Koelman <104792814+bart-vmware@users.noreply.github.com>

🎨 Fix casing in namespace

416957d

tscrypter added 5 commits April 15, 2025 00:11

restore PublicAPI.Unshipped.txt

a65eb25

start adding range header support

361636e

add public api changes

629afc2

fix some build errors

5c1f8aa

Use default Encoding for this .NET

5679892

bart-vmware reviewed Apr 15, 2025

View reviewed changes

tscrypter added 2 commits April 15, 2025 13:39

Merge branch 'main' into main

fefff15

Merge branch 'main' into main

f106e48

		options.AllowedVerbs.Add("Get");
		options.AllowedVerbs.Add("Head");

Add logfile actuator #1499

Are you sure you want to change the base?

Add logfile actuator #1499

Uh oh!

Conversation

tscrypter commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Quality checklist

Uh oh!

tscrypter commented Apr 3, 2025

Uh oh!

TimHess commented Apr 3, 2025

Uh oh!

tscrypter commented Apr 4, 2025

Uh oh!

bart-vmware 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!

Uh oh!

Uh oh!

Uh oh!

bart-vmware left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bart-vmware Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tscrypter commented Apr 9, 2025

Uh oh!

bart-vmware commented Apr 9, 2025

Uh oh!

bart-vmware left a comment

Choose a reason for hiding this comment

Uh oh!

bart-vmware Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

bart-vmware Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

bart-vmware Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

bart-vmware Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

bart-vmware Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

bart-vmware commented Apr 17, 2025

Uh oh!

azure-pipelines bot commented Apr 17, 2025

Uh oh!

sonarqubecloud bot commented Apr 17, 2025

Quality Gate passed

Uh oh!

bart-vmware commented May 2, 2025

Uh oh!

tscrypter commented May 2, 2025

Uh oh!

Uh oh!

tscrypter commented Apr 3, 2025 •

edited

Loading