Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

NaluTripician
Copy link
Contributor

@NaluTripician NaluTripician commented Feb 20, 2025

Pull Request Template

Description

Adds PPAF flag to user agent string when feature is enabled.

See #5027

Diagnostics example with ApplicationName:

{
    "Summary": {
        "DirectCalls": {
            "(200, 0)": 1
        },
        "GatewayCalls": {
            "(200, 0)": 2
        }
    },
    "name": "ReadItemAsync",
    "start datetime": "2025-02-20T20:49:57.316Z",
    "duration in milliseconds": 131.3269,
    "data": {
        "Client Configuration": {
            "Client Created Time Utc": "2025-02-20T20:49:57.3159592Z",
            "MachineId": "hashedMachineName:94d755e6-4bd9-6d68-c9d4-22b4d44d5b96",
            "NumberOfClientsCreated": 1,
            "NumberOfActiveClients": 1,
            "ConnectionMode": "Direct",
            "User Agent": "cosmos-netstandard-sdk/3.46.1|0|X64|Microsoft Windows 10.0.26100|.NET 6.0.36|S|ppaf|appName",
            "ConnectionConfig": {
                "gw": "(cps:50, urto:6, p:False, httpf: False)",
                "rntbd": "(cto: 5, icto: -1, mrpc: 30, mcpe: 65535, erd: True, pr: ReuseUnicastPort)",
                "other": "(ed:False, be:False)"
            },
            "ConsistencyConfig": "(consistency: NotSet, prgns:[West US 3, East US], apprgn: )",
            "ProcessorCount": 12
        }
    }
}

Diagnostics example without ApplicationName:

{
    "Summary": {
        "DirectCalls": {
            "(200, 0)": 1
        },
        "GatewayCalls": {
            "(200, 0)": 2
        }
    },
    "name": "ReadItemAsync",
    "start datetime": "2025-02-20T20:49:57.316Z",
    "duration in milliseconds": 131.3269,
    "data": {
        "Client Configuration": {
            "Client Created Time Utc": "2025-02-20T20:49:57.3159592Z",
            "MachineId": "hashedMachineName:94d755e6-4bd9-6d68-c9d4-22b4d44d5b96",
            "NumberOfClientsCreated": 1,
            "NumberOfActiveClients": 1,
            "ConnectionMode": "Direct",
            "User Agent": "cosmos-netstandard-sdk/3.46.1|0|X64|Microsoft Windows 10.0.26100|.NET 6.0.36|S|ppaf",
            "ConnectionConfig": {
                "gw": "(cps:50, urto:6, p:False, httpf: False)",
                "rntbd": "(cto: 5, icto: -1, mrpc: 30, mcpe: 65535, erd: True, pr: ReuseUnicastPort)",
                "other": "(ed:False, be:False)"
            },
            "ConsistencyConfig": "(consistency: NotSet, prgns:[West US 3, East US], apprgn: )",
            "ProcessorCount": 12
        }
    }
}

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Closing issues

To automatically close an issue: closes #5027

string suffix = string.Empty;
if (this.EnablePartitionLevelFailover)
{
suffix += string.IsNullOrEmpty(this.ApplicationName) ? "ppaf" : "ppaf|";
Copy link
Member

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)
Copy link
Member

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.

Copy link
Member

@kundadebdatta kundadebdatta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@kundadebdatta kundadebdatta added the auto-merge Enables automation to merge PRs label Feb 21, 2025
@kundadebdatta
Copy link
Member

@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"
Copy link
Member

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?

Copy link
Member

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

Copy link
Member

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)

Copy link
Contributor Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Enables automation to merge PRs PerPartitionAutomaticFailover
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Per Partition Automatic Failover] - Update the Cosmos Diagnostics to Include Flags Related to PPAF and PPCB
4 participants