-
Notifications
You must be signed in to change notification settings - Fork 501
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Internal] PPAF: Adds PPAF information to diagnostics #5028
base: master
Are you sure you want to change the base?
Conversation
string suffix = string.Empty; | ||
if (this.EnablePartitionLevelFailover) | ||
{ | ||
suffix += string.IsNullOrEmpty(this.ApplicationName) ? "ppaf" : "ppaf|"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use interpolated string instead of concatenation ?
[TestMethod] | ||
[DataRow(true, DisplayName = "With ApplicationName")] | ||
[DataRow(false, DisplayName = "Without ApplicationName")] | ||
public void UserAgentContainsPPAFInformation(bool appName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Let's add [Owner("")]
section as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@NaluTripician : I think it would be great if you could attach a section of the diagnostics, with the new changes in the user agent. |
|
||
string suffix = this.EnablePartitionLevelFailover | ||
? string.IsNullOrEmpty(this.ApplicationName) | ||
? "ppaf" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UserAgent prefix length has limitation. How about limit it to a single character?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would actually prefer us agreeing on a flags enum (bit mask) - like for SDK-Capabilities. We can use the same bit mask in Java (when featuresa re applicable both) - this would make it easier to consume this in Kusto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so something like |F7| (for Features of bit 1, 2 and 4)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change to F7 to have it consistent with Java. For when PPCB is added, what would the flag be for when both ppaf and ppcb are enabled?
Pull Request Template
Description
Adds PPAF flag to user agent string when feature is enabled.
See #5027
Diagnostics example with ApplicationName:
Diagnostics example without ApplicationName:
Type of change
Please delete options that are not relevant.
Closing issues
To automatically close an issue: closes #5027