Add telemetry values to auth request and logout URIs, along with a sw… #531
Conversation
|
There is some feedback from the community that OFF by default seems to be preferred. |
| @@ -165,15 +165,14 @@ public void GetSets() | |||
| OpenIdConnectMessage message = new OpenIdConnectMessage(); | |||
| Type type = typeof(OpenIdConnectMessage); | |||
| PropertyInfo[] properties = type.GetProperties(); | |||
| if (properties.Length != 47) | |||
| if (properties.Length != 48) | |||
| Assert.True(true, "Number of public fields has changed from 47 to: " + properties.Length + ", adjust tests"); | |||
Are you sure it was accidental? |
|
@brentschmaltz BTW you can un-thumbs-up by clicking the 👍 icon at the bottom of the issue again. |
polita
force-pushed
the
polita/498Wilson5x
branch
from
6ac919e to
da53b3b
Compare
|
@PinpointTownes :-) always a pleasure. |
polita
force-pushed
the
polita/498Wilson5x
branch
from
da53b3b to
78cca8c
Compare
|
@brentschmaltz Can you CR? |
| /// Gets or sets whether parameters for the library and version are sent on the query string for this <see cref="OpenIdConnectMessage"/> instance. | ||
| /// This value is set to the value of EnableTelemetryParametersByDefault at message creation time. | ||
| /// </summary> | ||
| public bool EnableTelemetryParameters |
There was a problem hiding this comment.
The only thing I can think of is if we did
{get;set;} = EnableTelemetryParametersByDefault;
would remove the need for a few sets in some methods.
There was a problem hiding this comment.
@polita I don't see this in current PR. I was thinking this should be on the Property EnableTelemetryParameters
|
I don't wanna be evil, but note that thanks to the "lowest applicable version" policy used by NuGet, folks using the OIDC middleware won't get telemetry until a new OIDC package is released, unless they explicitly specify the latest IdentityModel version in their project.json 😄 ... so maybe a static property is not necessary...? |
|
@PinpointTownes Yes, it's true that you'd have to explicitly download the new package. But since people might be pulling down a new package for other reasons than the awesome new telemetry feature, giving them a single line of code way to turn it off is still desirable. At least, I thought for sure you'd be a proponent of that... :) |
polita
force-pushed
the
polita/498Wilson5x
branch
from
78cca8c to
8e152ab
Compare
|
@brentschmaltz Thanks- updated the CR and squashed. |
polita
force-pushed
the
polita/498Wilson5x
branch
from
8e152ab to
e81ca05
Compare
|
@brentschmaltz Updated with the change to use auto-property initializers. |
| /// <summary> | ||
| /// Gets or sets whether parameters for the library and version are sent on the query string for all instances of <see cref="OpenIdConnectMessage"/>. | ||
| /// </summary> | ||
| public static bool EnableTelemetryParametersByDefault = true; |
There was a problem hiding this comment.
Out of curiosity, why not a property? 😮
There was a problem hiding this comment.
What would a property getter/setter get you that this doesn't?
There was a problem hiding this comment.
The immense satisfaction of respecting the best practices? 
Exposing public fields is generally discouraged (and I'm pretty sure there's even an FxCop rule for that)
There was a problem hiding this comment.
Property implementations can be changed later without breaking callers. Fields can be changed to properties later but it requires recompiling the caller.
There was a problem hiding this comment.
@Tratcher are you thinking something like, we set the property from an external source (config, etc)? Where the property could have some smarts?
There was a problem hiding this comment.
Yes, that's the sort of future modification that may happen for an API like this. If it's a property then you can change it, but if it's a field you can't.
There was a problem hiding this comment.
Updated to make it a property. Thanks for the feedback.
…itch to turn telemetry off Update message in test Update PR based on new design to include a static switch to easily turn off telemetry Update the getter/setter syntax with CR feedback Update with CR feedback on syntax Update with CR feedback
polita
force-pushed
the
polita/498Wilson5x
branch
from
e81ca05 to
7c8ab45
Compare
|
Well that was easy :-) |
…itch to turn telemetry off