Trace Actuator Updates #1356

TimHess · 2024-09-12T21:54:06Z

Description

While adding support for header filtering, I discovered we're a bit out of sync with Spring on this actuator in general, so I also decided to:

Remove everything related to the /trace (v1) actuator
Rename httptrace to httpexchanges (to align with Spring Boot 3), but still use /httptrace on TPCF

I also ran into an unhandled exception with the mappings actuator and included a fix for that

Fixes #1351

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.

TimHess · 2024-09-13T12:59:16Z

/azp run cleanup-code

azure-pipelines · 2024-09-13T12:59:27Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2024-09-13T13:03:39Z

Code cleanup failed to reformat and push changes.

View details here.

bart-vmware

First glance over the changes. It's pretty hard to see what's changed on GitHub due to renames. I'll run a file-by-file diff in the next round.

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

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchange.cs

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchangesEndpointOptions.cs

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchangesDiagnosticObserver.cs

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchangesEndpointOptions.cs

src/Management/src/Endpoint/ConfigurationSchema.json

src/Management/src/Endpoint/EndpointOptionsExtensions.cs

src/Management/src/Endpoint/ManagementWebHostBuilderExtensions.cs

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

src/Management/src/Endpoint/Actuators/HttpExchanges/ConfigureHttpExchangesEndpointOptions.cs

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchangesDiagnosticObserver.cs

bart-vmware · 2024-09-17T11:36:28Z

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchangesDiagnosticObserver.cs

+        return new HttpExchangesResult(recentExchanges);
+    }
+
+    public override void ProcessEvent(string eventName, object? value)


Can we just type-check value for HttpContext instead of the reflection-based properly lookups? Same for the other two observers. I suspect this is a leftover from the split between base/core and support for the full .NET framework.

I think we should drop HttpClientDesktopObserver (which reads from WebRequest) entirely, because it was obsoleted in .NET 6.

Then rename the ~~renaming~~ remaining one to just HttpClientObserver.

value is an anonymous type with a HttpContext as a property... I can't seem to get type checking to work in this scenario and haven't found anything in several web searches.

You're probably right about the other observers, I'm not sure yet if it's something I want to include in this PR though

value is an anonymous type with a HttpContext as a property... I can't seem to get type checking to work in this scenario and haven't found anything in several web searches.

These are just the tests acting silly by passing an anonymous type. As can be seen here, ASP.NET always passes in a HttpContext instance. So the code can just be:

if (value is HttpContext httpContext) { RecordHttpExchange(current, httpContext); }

When using that and debugging the test AddSeveralActuators_WebApplicationBuilder_NoConflict (breakpoint in method HttpExchangesDiagnosticObserver.ProcessEvent), it always sends an HttpContext instance.

You're probably right about the other observers, I'm not sure yet if it's something I want to include in this PR though

Why not, do you have any concerns? The current names are just confusing. The other observers suffer from the same issues as addressed in this PR, such as obtaining Activity.Current when not needed. Please open an issue otherwise.

These are just the tests acting silly by passing an anonymous type...

I should know by now not to trust that these tests are always doing the right thing 🙄

Why not, do you have any concerns? The current names are just confusing. The other observers suffer from the same issues as addressed in this PR, such as obtaining Activity.Current when not needed. Please open an issue otherwise.

Not any specific concerns, more a question of maintaining focus... If I have time to tackle it before your next review I will, otherwise I'll create a follow-up issue so this PR doesn't keep dragging on

I can do the delete & rename, but it looks like we can't currently adjust the reflection-based property access code: dotnet/runtime#82910

I don't understand what's going on. The code was updated to type-check for HttpContext as I suggested, and now only unit tests call the method GetHttpContextPropertyValue (so it has basically become redundant). But the runtime issue states we still need to use reflection?

Oh right... I didn't come back and check on other usages of GetHttpContextPropertyValue after trying to adjust the other observers. I'll remove this method and the two tests. The runtime issue is the reason why I can't do the same for the HttpClient Observer(s)

Okay, I understand that HttpClientObserver needs to keep using reflection due to the runtime issue. But that doesn't apply to AspNetCoreHostingObserver, so please update to remove the reflection there.

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchangesResult.cs

src/Management/src/Endpoint/Actuators/HttpExchanges/ServiceCollectionExtensions.cs

src/Management/src/Endpoint/Actuators/RouteMappings/RouteMappingsEndpointHandler.cs

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

src/Management/test/Endpoint.Test/ManagementWebApplicationBuilderExtensionsTest.cs

TimHess · 2024-09-18T23:19:05Z

/azp run cleanup-code

azure-pipelines · 2024-09-18T23:19:13Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2024-09-18T23:27:44Z

Code cleanup successfully reformatted files and pushed changes.

TimHess · 2024-09-19T15:30:05Z

/azp run cleanup-code

azure-pipelines · 2024-09-19T15:30:17Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2024-09-19T15:38:12Z

Code cleanup successfully reformatted files and pushed changes.

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchangesDiagnosticObserver.cs

src/Management/src/Endpoint/EndpointOptionsExtensions.cs

src/Management/src/Endpoint/Actuators/HttpExchanges/ConfigureHttpExchangesEndpointOptions.cs

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchangesDiagnosticObserver.cs

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchange.cs

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchangesDiagnosticObserver.cs

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchangesEndpointOptions.cs

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

bart-vmware

I've completed a full sweep of all changes. Please don't rebase anymore until this PR is approved.

src/Management/src/Endpoint/Actuators/HttpExchanges/ServiceCollectionExtensions.cs

src/Management/src/Endpoint/ManagementHostBuilderExtensions.cs

src/Management/src/Endpoint/ManagementHostApplicationBuilderExtensions.cs

src/Management/src/Endpoint/ManagementWebHostBuilderExtensions.cs

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchangesDiagnosticObserver.cs

src/Management/src/Endpoint/Actuators/HttpExchanges/ConfigureHttpExchangesEndpointOptions.cs

bart-vmware · 2024-09-23T10:58:39Z

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchangesDiagnosticObserver.cs

+        return new HttpExchangesResult(recentExchanges);
+    }
+
+    public override void ProcessEvent(string eventName, object? value)


I don't understand what's going on. The code was updated to type-check for HttpContext as I suggested, and now only unit tests call the method GetHttpContextPropertyValue (so it has basically become redundant). But the runtime issue states we still need to use reflection?

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchangesDiagnosticObserver.cs

Co-authored-by: Bart Koelman <[email protected]>

TimHess · 2024-09-23T21:53:51Z

/azp run Steeltoe.All

azure-pipelines · 2024-09-23T21:53:59Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2024-09-23T23:27:14Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2024-09-23T23:35:05Z

Code cleanup successfully reformatted files and pushed changes.

bart-vmware · 2024-09-24T08:12:08Z

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

+    [Fact]
+    public async Task HttpExchangesActuator_DoesNotCaptureAuthInUri()
+    {
+        WebApplicationBuilder builder = TestWebApplicationBuilderFactory.Create();
+        builder.Configuration.AddInMemoryCollection(AppSettings);
+        builder.AddHttpExchangesActuator();
+        await using WebApplication host = builder.Build();
+
+        host.UseRouting();
+        await host.StartAsync();
+        using HttpClient httpClient = host.GetTestClient();
+
+        var requestUri = new Uri("http://username:password@localhost/actuator/httpexchanges");
+        _ = await httpClient.GetAsync(requestUri);
+        HttpResponseMessage response = await httpClient.GetAsync(requestUri);
+        response.StatusCode.Should().Be(HttpStatusCode.OK);
+
+        string json = await response.Content.ReadAsStringAsync();
+        json.Should().NotContain("username").And.NotContain("password");
+        json.Should().Contain("http://localhost:80/actuator/httpexchanges");
+    }


Suggested change

[Fact]

public async Task HttpExchangesActuator_DoesNotCaptureAuthInUri()

{

WebApplicationBuilder builder = TestWebApplicationBuilderFactory.Create();

builder.Configuration.AddInMemoryCollection(AppSettings);

builder.AddHttpExchangesActuator();

await using WebApplication host = builder.Build();

host.UseRouting();

await host.StartAsync();

using HttpClient httpClient = host.GetTestClient();

var requestUri = new Uri("http://username:password@localhost/actuator/httpexchanges");

_ = await httpClient.GetAsync(requestUri);

HttpResponseMessage response = await httpClient.GetAsync(requestUri);

response.StatusCode.Should().Be(HttpStatusCode.OK);

string json = await response.Content.ReadAsStringAsync();

json.Should().NotContain("username").And.NotContain("password");

json.Should().Contain("http://localhost:80/actuator/httpexchanges");

}

[Fact]

public async Task HttpExchangesActuator_DoesNotCaptureAuthInUri()

{

WebApplicationBuilder builder = TestWebApplicationBuilderFactory.CreateDefault(false);

builder.Configuration.AddInMemoryCollection(AppSettings);

builder.AddHttpExchangesActuator();

await using WebApplication host = builder.Build();

host.UseRouting();

host.MapGet("/hello", () => "Hello World!");

await host.StartAsync();

using var httpClient = new HttpClient();

HttpResponseMessage helloResponse = await httpClient.GetAsync("http://username:password@localhost:5000/hello");

helloResponse.StatusCode.Should().Be(HttpStatusCode.OK);

HttpResponseMessage actuatorResponse = await httpClient.GetAsync(new Uri("http://localhost:5000/actuator/httpexchanges"));

actuatorResponse.StatusCode.Should().Be(HttpStatusCode.OK);

var actuatorRootElement = await actuatorResponse.Content.ReadFromJsonAsync<JsonElement>();

JsonElement[] exchangesArray = actuatorRootElement.GetProperty("exchanges").EnumerateArray().ToArray();

exchangesArray.Should().HaveCount(1);

string requestUri = exchangesArray[0].GetProperty("request").GetProperty("uri").ToString();

requestUri.Should().Be("http://localhost:5000/hello");

}

The suggested test is more reliable because:

It starts a real server, bypassing short-circuiting from TestServer.

It verifies a regular request, instead of an actuator endpoint.

It verifies we're actually getting back a response that's not an empty exchanges array.

It verifies the URI is correct, not empty/omitted, etc.

Note: Taking this suggestion requires adding using System.Text.Json; at the top.

bart-vmware · 2024-09-24T08:18:32Z

...Management/test/Endpoint.Test/Actuators/HttpExchanges/HttpExchangesDiagnosticObserverTest.cs

+        filteredHeaders.Response.Headers["HeaderA"][0].Should().Be(HttpExchangesDiagnosticObserver.Redacted);
+        filteredHeaders.Response.Headers["HeaderB"][0].Should().Be("headerBValue");
+        filteredHeaders.Response.Headers[HeaderNames.SetCookie][0].Should().Be(HttpExchangesDiagnosticObserver.Redacted);
+        filteredHeaders.Principal?.Name.Should().Be("MyTestName");


Never use ?. in FluentAssertions-style assertions. If null, the right-hand side doesn't execute, thus no assertion is performed.

Aside from multiple occurrences in this file, please also correct the ones in ApplicationInstanceCertificateTest.

bart-vmware · 2024-09-24T08:36:40Z

...Management/test/Endpoint.Test/Actuators/HttpExchanges/HttpExchangesDiagnosticObserverTest.cs

+            observer.ProcessEvent("Microsoft.AspNetCore.Hosting.HttpRequestIn.Stop", context);
+        }
+
+        observer.Queue.Count.Should().Be(option.CurrentValue.Capacity);


This is a poor assertion, because it won't dump the collection contents on failure. The best it can show is "5 should be 4".
Instead, use:

observer.Queue.Should().HaveCount(option.CurrentValue.Capacity);

Applies to multiple places in this file.

bart-vmware · 2024-09-24T08:38:06Z

...Management/test/Endpoint.Test/Actuators/HttpExchanges/HttpExchangesDiagnosticObserverTest.cs

+    {
+        using var listener = new DiagnosticListener("test");
+
+        IOptionsMonitor<HttpExchangesEndpointOptions> option =


Please rename these vars to optionsMonitor, which is what they are.

...Management/test/Endpoint.Test/Actuators/HttpExchanges/HttpExchangesDiagnosticObserverTest.cs

bart-vmware · 2024-09-24T09:12:50Z

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchange.cs

+
+public sealed class HttpExchange
+{
+    [JsonPropertyName("TimeTaken")]


Why is this Pascal-cased, whereas all other properties aren't?
And this doesn't actually work because the JSON serializer skips internal properties. Test coverage is missing. And apparently, it hasn't been manually tested either.

Having unit tests is no excuse for omitting tests that verify the entire feature works, which was already identified in #1028. What ultimately matters the most is the response being returned, because that's what users observe. So at the minimum, there must be a test that asserts on the JSON returned when all options are turned on, and one for when all options are turned off. As they are still missing (this far in the review process), I wrote them myself in aaf76b6. In doing so, I've identified various bugs.

Sorry, that was a combination of copy/paste from the property name and being distracted by time stamp in the response without paying close enough attention. Thanks for writing the tests, I pulled in that commit

bart-vmware · 2024-09-24T11:07:00Z

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchange.cs

+
+public sealed class HttpExchange
+{
+    public DateTime Timestamp { get; }


This is still a UTC date/time in the response, not a timestamp in Unix Epoch ticks.

bart-vmware · 2024-09-24T11:13:59Z

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

+        response = await httpClient.GetAsync(new Uri("http://localhost/actuator/httpexchanges"));
+        response.StatusCode.Should().Be(HttpStatusCode.OK);
+        string json = await response.Content.ReadAsStringAsync();
+        json.Should().NotContain("username").And.NotContain("password");


This doesn't make sense, there's no username/password anywhere in this test.

bart-vmware · 2024-09-24T11:14:40Z

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

+        json.Should().Contain("http://localhost:80/actuator/httpexchanges");
+        json.Should().Contain($"\"timestamp\":\"{DateTime.UtcNow.Date:yyyy-MM-dd}");


These assertions are way insufficient, see my other comment.

This test and the one above now effectively do the same thing but with different assertions, so I'm just going to consolidate them and fill out the assertion set for all of the properties

bart-vmware · 2024-09-24T11:17:49Z

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchangesDiagnosticObserver.cs

+        return new HttpExchangesResult(recentExchanges);
+    }
+
+    public override void ProcessEvent(string eventName, object? value)


Okay, I understand that HttpClientObserver needs to keep using reflection due to the runtime issue. But that doesn't apply to AspNetCoreHostingObserver, so please update to remove the reflection there.

…re/better assertions

azure-pipelines · 2024-09-24T21:13:58Z

Azure Pipelines successfully started running 1 pipeline(s).

sonarcloud · 2024-09-24T21:29:44Z

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 SonarCloud

TimHess added Type/enhancement New feature or request Component/Management Issues related to Steeltoe Management (actuators) ReleaseLine/4.x Identified as a feature/fix for the 4.x release line labels Sep 12, 2024

TimHess added this to the 4.0.0-m1 milestone Sep 12, 2024

TimHess self-assigned this Sep 12, 2024

TimHess force-pushed the feature/header-masking-httptrace branch from a51e47c to ba2fd37 Compare September 13, 2024 13:57

bart-vmware reviewed Sep 13, 2024

View reviewed changes

TimHess force-pushed the feature/header-masking-httptrace branch 3 times, most recently from 3609088 to 27ff773 Compare September 16, 2024 20:15

bart-vmware requested changes Sep 17, 2024

View reviewed changes

bart-vmware mentioned this pull request Sep 17, 2024

Configuration schema generator: fix missing cref inside para tag #1358

Merged

5 tasks

TimHess force-pushed the feature/header-masking-httptrace branch 2 times, most recently from c25ef15 to 8d78612 Compare September 18, 2024 22:16

TimHess changed the base branch from main to fix-schema-cref-in-para September 18, 2024 22:16

TimHess requested a review from bart-vmware September 18, 2024 23:36

TimHess marked this pull request as ready for review September 19, 2024 13:48

bart-vmware closed this in #1358 Sep 19, 2024

TimHess reopened this Sep 19, 2024

TimHess changed the base branch from fix-schema-cref-in-para to main September 19, 2024 14:06

TimHess force-pushed the feature/header-masking-httptrace branch from df33a65 to 38337b9 Compare September 19, 2024 15:47

bart-vmware reviewed Sep 20, 2024

View reviewed changes

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchangesDiagnosticObserver.cs Outdated Show resolved Hide resolved

src/Management/src/Endpoint/Actuators/HttpExchanges/HttpExchangesEndpointOptions.cs Outdated Show resolved Hide resolved

bart-vmware requested changes Sep 20, 2024

View reviewed changes

src/Management/test/Endpoint.Test/Actuators/CloudFoundry/EndpointMiddlewareTest.cs Outdated Show resolved Hide resolved

This was referenced Sep 20, 2024

Better support for Minimal Api in /mappings #1366

Open

Revisit Actuator MediaTypeVersion handling #1367

Open

TimHess added 2 commits September 20, 2024 15:49

Replace /trace and /httptrace with SpringBoot3-compatible /httpexchanges

154493c

drop HttpClientDesktopObserver

42752ce

TimHess force-pushed the feature/header-masking-httptrace branch from 5955f36 to 42752ce Compare September 20, 2024 20:56

TimHess requested a review from bart-vmware September 20, 2024 20:57

bart-vmware requested changes Sep 23, 2024

View reviewed changes

TimHess and others added 5 commits September 23, 2024 09:37

Apply suggestions from code review

bcf8c27

Co-authored-by: Bart Koelman <[email protected]>

Apply suggestions from code review

025df11

Co-authored-by: Bart Koelman <[email protected]>

Apply suggestions from code review

7e31de4

Co-authored-by: Bart Koelman <[email protected]>

PR feedback

97af1c5

Use DateTime.*Utc*Now.Date

ec98cea

Automated code cleanup from Steeltoe cibuild

0f72484

Add HTTP exchange test that asserts on JSON structure

aaf76b6

bart-vmware requested changes Sep 24, 2024

View reviewed changes

update api spec, remove reflection from AspNetCoreHostingObserver, mo…

b036ab2

…re/better assertions

TimHess requested a review from bart-vmware September 24, 2024 21:23

		json.Should().Contain("http://localhost:80/actuator/httpexchanges");
		json.Should().Contain($"\"timestamp\":\"{DateTime.UtcNow.Date:yyyy-MM-dd}");

Trace Actuator Updates #1356

Are you sure you want to change the base?

Trace Actuator Updates #1356

Conversation

TimHess commented Sep 12, 2024 • edited Loading

Description

Quality checklist

TimHess commented Sep 13, 2024

azure-pipelines bot commented Sep 13, 2024

azure-pipelines bot commented Sep 13, 2024

bart-vmware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bart-vmware Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bart-vmware Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimHess commented Sep 18, 2024

azure-pipelines bot commented Sep 18, 2024

azure-pipelines bot commented Sep 18, 2024

TimHess commented Sep 19, 2024 • edited Loading

azure-pipelines bot commented Sep 19, 2024

azure-pipelines bot commented Sep 19, 2024

bart-vmware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimHess commented Sep 23, 2024 • edited Loading

azure-pipelines bot commented Sep 23, 2024

azure-pipelines bot commented Sep 23, 2024

azure-pipelines bot commented Sep 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azure-pipelines bot commented Sep 24, 2024

sonarcloud bot commented Sep 24, 2024

Quality Gate passed

TimHess commented Sep 12, 2024 •

edited

Loading

bart-vmware Sep 17, 2024 •

edited

Loading

bart-vmware Sep 20, 2024 •

edited

Loading

TimHess commented Sep 19, 2024 •

edited

Loading

TimHess commented Sep 23, 2024 •

edited

Loading