Improve endpoint metadata debugging #47163

SeanFarrow · 2023-03-12T10:01:59Z

{PR title}

This PR improves endpoint metadata debugging.

Description

This PR overrides ToString where necessary to improve endpoint metadata debugging.

Fixes #39792

ghost · 2023-03-12T10:02:06Z

Thanks for your PR, @SeanFarrow. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

SeanFarrow · 2023-03-12T10:04:22Z

This is not ready yet, as there is a build error due to the fact that the CodeFix that adds methods to the public API has added the ToString method of the EndpointMetadataCollection class to the wrong place.

I still need to finish implement ToString for both the Endpoint base class and RouteEndpoint class.

adityamandaleeka · 2023-03-13T22:01:23Z

Thanks @SeanFarrow! I've marked this as a draft since it's not ready yet. Feel free to mark it ready when appropriate.

@JamesNK Assigning this one to you for review.

JamesNK · 2023-03-14T00:58:29Z

src/Http/Routing/src/DataTokensMetadata.cs

+        var metadataTokensBuilder = new StringBuilder();
+        foreach(var DataToken in DataTokens)
+        {
+            metadataTokensBuilder.AppendFormat(CultureInfo.InvariantCulture, "{0} ={1}", DataToken.Key, DataToken.Value);
+            metadataTokensBuilder.AppendLine();
+        }
+            return metadataTokensBuilder.ToString();


I prefer comma delimited rather than separate lines. Separate lines can mess up tooling.
Also, include text to label what the data is.

Something like:

var dataTokensDisplay = string.Join(",", DataTokens.Select(t => $"{t.Key}={t.Value}")); return $"DataTokens: {dataTokensDisplay}";

JamesNK · 2023-03-14T01:00:06Z

src/Http/Routing/src/EndpointNameMetadata.cs

+    /// <inheritdoc/>
+    public override string ToString()
+    {
+        return EndpointName;


Suggested change

return EndpointName;

return $"EndpointName: {EndpointName}";

JamesNK · 2023-03-14T01:00:49Z

src/Http/Routing/src/SuppressLinkGenerationMetadata.cs

+    /// <inheritdoc/>
+    public override string ToString()
+    {
+        return SuppressLinkGeneration.ToString();


Suggested change

return SuppressLinkGeneration.ToString();

return $"SuppressLinkGeneration = {SuppressLinkGeneration}";

JamesNK · 2023-03-14T01:01:16Z

src/Http/Routing/src/SuppressMatchingMetadata.cs

+    /// <inheritdoc/>
+    public override string ToString()
+    {
+        return SuppressMatching.ToString();


Suggested change

return SuppressMatching.ToString();

return $"SuppressMatching = {SuppressMatching}";

JamesNK · 2023-03-14T01:15:31Z

src/Http/Http.Abstractions/src/Routing/EndpointMetadataCollection.cs

@@ -156,6 +157,24 @@ public EndpointMetadataCollection(params object[] items)
    /// <returns>An <see cref="IEnumerator"/> of all metadata items.</returns>
    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();

+    /// <inheritdoc/>
+    public override string ToString()


I don't think we should override this collection ToString. If someone wants to view the contents then they can do it with a debugger.

Instead, put a debugger display on the collection:

[DebuggerDisplay("Count = {Count}")]

If you want, you could also add a debugger proxy. List<T> does this for example: https://github.com/dotnet/runtime/blob/73818601becad0d1c6cfc58b5d947db010da2d6b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/List.cs#L17-L18. A debugger proxy changes what properties are visible in IDEs when debugging.

You'll need to copy and paste this internal type: https://github.com/dotnet/runtime/blob/73818601becad0d1c6cfc58b5d947db010da2d6b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ICollectionDebugView.cs
Put it under src/Http/Http.Abstractions/src/Internal and update the namespace to match other files in that directory.

JamesNK · 2023-03-14T01:16:49Z

Looking good so far. I think these changes will make debugging endpoint metadata much easier.

javiercn

I'm ok if we want to do this change, but I would avoid overriding ToString and instead use [DebuggerDisplay] and a private method on the types.

halter73 · 2023-03-27T21:43:48Z

I'm ok if we want to do this change, but I would avoid overriding ToString and instead use [DebuggerDisplay] and a private method on the types.

This was discussed in API review, and we went with ToString(). It was considered in the initial proposal under alternative designs.

DebuggerDisplayAttribute is not appropriate because there are situations where it is useful to get the debug value at runtime. e.g. logging.

JamesNK · 2023-05-03T01:55:12Z

@SeanFarrow What are your plans for this PR? If you don't have time right now to complete it then I can take it over.

SeanFarrow · 2023-05-03T02:21:42Z

@JamesNK I'm going to pick this up again next week--I am in the process of changing jobs, so have 2 weeks off before the next one starts! Hopefully this is ample time to get this finished.

…than specifically implementing aDebuggerToString method.

…emove the DebuggerToString method.

… in the RouteNameMetadata class.

…ass.

…the use of StringBuilder.

SeanFarrow · 2023-05-11T18:17:54Z

@JamesNK Are you able to pick this up please.

I thought I'd have more time than I have, but this won't be the case--at least not before 8.0, given my mum is not well and I'm changing jobs!

Thanks,
Sean.

JamesNK · 2023-05-11T23:28:18Z

Sure, I can finish this off. Thanks for starting.

JamesNK · 2023-05-12T07:17:21Z

Replaced by #48207

SeanFarrow requested review from javiercn, Tratcher, BrennanConroy, halter73 and captainsafia as code owners March 12, 2023 10:02

ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels Mar 12, 2023

mkArtakMSFT assigned adityamandaleeka Mar 13, 2023

adityamandaleeka marked this pull request as draft March 13, 2023 21:59

adityamandaleeka assigned JamesNK and unassigned adityamandaleeka Mar 13, 2023

JamesNK reviewed Mar 14, 2023

View reviewed changes

build-analysis bot mentioned this pull request Mar 14, 2023

Request failed "500 Internal Server Error" dotnet/arcade#11737

Open

5 tasks

javiercn reviewed Mar 27, 2023

View reviewed changes

SeanFarrow added 5 commits May 10, 2023 08:50

Override the ToString method in the DataTokensMetadata class.

bbd0058

Override the ToString method in the EndpointNameMetadata class.

6252be4

Override the ToString method in the HostAttribute class.

9bcccae

Use the overridden ToString method of the HostAttribute class rather …

ae62eec

…than specifically implementing aDebuggerToString method.

Override the ToString method in the HttpMethodMetadata class.

c622400

SeanFarrow added 11 commits May 10, 2023 08:50

Use the ToString method of the HttpMethodMetadata for debugging and r…

2810c06

…emove the DebuggerToString method.

Override the ToString method in the RouteNameMetadata class.

b4e3786

Use the newly implemented ToString method instead of DebuggerToString…

19e069c

… in the RouteNameMetadata class.

Override the ToString method in the SuppressLinkGenerationMetadata cl…

a388d8e

…ass.

Override the ToString method in the SuppressMatchingMetadata class.

3cf3bb1

Update the ToString method of the DataTokensMetadata class to remove …

1dcbd3f

…the use of StringBuilder.

Override the ToString method in the DataTokensMetadata class.

9000058

Override the ToString method in the HostAttribute class.

fc093b2

Override the ToString method in the HttpMethodMetadata class.

1c2cc7f

Override the ToString method in the RouteNameMetadata class.

17ec7f6

Update the ToString method of the DataTokensMetadata class to remove …

4f2b05a

…the use of StringBuilder.

JamesNK mentioned this pull request May 12, 2023

Improve endpoint metadata debugging #48207

Merged

JamesNK closed this May 12, 2023

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023

	return SuppressLinkGeneration.ToString();
	return $"SuppressLinkGeneration = {SuppressLinkGeneration}";

	return SuppressMatching.ToString();
	return $"SuppressMatching = {SuppressMatching}";

Improve endpoint metadata debugging #47163

Improve endpoint metadata debugging #47163

Uh oh!

Conversation

SeanFarrow commented Mar 12, 2023

{PR title}

Description

Uh oh!

ghost commented Mar 12, 2023

Uh oh!

SeanFarrow commented Mar 12, 2023

Uh oh!

adityamandaleeka commented Mar 13, 2023

Uh oh!

JamesNK Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

JamesNK Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

JamesNK Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

JamesNK commented Mar 14, 2023

Uh oh!

javiercn left a comment

Choose a reason for hiding this comment

Uh oh!

halter73 commented Mar 27, 2023

Uh oh!

JamesNK commented May 3, 2023

Uh oh!

SeanFarrow commented May 3, 2023

Uh oh!

SeanFarrow commented May 11, 2023

Uh oh!

JamesNK commented May 11, 2023

Uh oh!

JamesNK commented May 12, 2023

Uh oh!

Uh oh!

JamesNK Mar 14, 2023 •

edited

Loading

JamesNK Mar 14, 2023 •

edited

Loading