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

Fix activity correlator to continue using same GUID for connection activity #1997

Merged
merged 1 commit into from
Apr 17, 2023

Conversation

cheenamalhotra
Copy link
Member

@cheenamalhotra cheenamalhotra commented Apr 14, 2023

I learnt about this issue internally that Activity Id Correlation was not working properly, and Activity Id GUID was not being reused when connecting to Azure SQL with 'redirection', as that is expected for the same connection activity.
Activity Id should be retained for the physical connection as is also the case with the ODBC driver with sequence increments.

Below code can be used to test the bug and verify the fix:

// EXPECTED: When pooling is disabled, 2 physical connections are made, activity Id should be different 
// for both connections but should be same within the redirect flow when performing 2 prelogin handshakes.
// conn1: ClientConnectionID 804265c3-79b1-4c6c-8392-678226b4ec46, ActivityID 1aadd57b-c181-489f-a0d4-5ae9c3f50c86:1
// conn2: ClientConnectionID d535123b-a74b-4731-a08d-abe098dea191, ActivityID 25c04c57-d9b5-4720-ade7-dc6a9dbd3852:1
// conn1: ClientConnectionID fa1f0881-6eaa-4688-9d66-6d22dafbf1c0, ActivityID 1aadd57b-c181-489f-a0d4-5ae9c3f50c86:2
// conn2: ClientConnectionID 58309c6d-56d3-4d35-9cd0-0b1f01347985, ActivityID 25c04c57-d9b5-4720-ade7-dc6a9dbd3852:2

// EXPECTED: When pooling is enabled, 1 physical connection is made, activity Id is same for all prelogin handshakes.
// conn1: ClientConnectionID 04ca1d05-039b-4318-b852-1404edf8c9dc, ActivityID 7fe62ff5-82bd-4c5f-8773-652e3e3e2ccb:1
// conn1: ClientConnectionID e8da8405-0fd7-4b4a-81c1-d4f142c0a3e7, ActivityID 7fe62ff5-82bd-4c5f-8773-652e3e3e2ccb:2
// conn2: ClientConnectionID e3c8823e-077e-498d-83e3-aa1f4f54518e, ActivityID 7fe62ff5-82bd-4c5f-8773-652e3e3e2ccb:3
// conn2: ClientConnectionID 09e83e05-c442-4a3f-b0b3-ea5501606edb, ActivityID 7fe62ff5-82bd-4c5f-8773-652e3e3e2ccb:4

const string CONN_STRING= "Server=*****.database.windows.net; Initial Catalog=***; UID=***; PWD=***; Pooling=false;";

static async Task Main(string[] args)
{
    using SqlClientListener listener = new();
    await RunTest(); // conn1
    await RunTest(); // conn2
}

static async Task RunTest()
{
    using SqlConnection conn1 = new(CONN_STRING);
    await conn1.OpenAsync();
}

public class SqlClientListener : EventListener
{
    protected override void OnEventSourceCreated(EventSource eventSource)
    {
        // Only enable events from SqlClientEventSource.
        if (eventSource.Name.Equals("Microsoft.Data.SqlClient.EventSource"))
        {
            EnableEvents(eventSource, EventLevel.Informational, EventKeywords.All);
        }
    }

    /// <summary>
    /// This callback runs whenever an event is written by SqlClientEventSource.
    /// </summary>
    /// <param name="eventData">Event data captured.</param>
    protected override void OnEventWritten(EventWrittenEventArgs eventData)
    {
        if (eventData.Payload == null)
        {
            return;
        }
        foreach (object payload in eventData.Payload)
        {
            if (payload != null)
            {
                Console.WriteLine(payload.ToString());
            }
        }
    }
}

When looking into traces, notice the activity Id associated with <sc.TdsParser.SendPreLoginHandshake|INFO> event.

Capturing activity Id correlated with connection activity is helpful for troubleshooting purposes.

@cheenamalhotra cheenamalhotra changed the title Fix activity correlator to continue using same GUID for current thread activity Fix activity correlator to continue using same GUID for connection activity Apr 14, 2023
@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.99 ⚠️

Comparison is base (4702420) 70.83% compared to head (b4136b5) 69.84%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1997      +/-   ##
==========================================
- Coverage   70.83%   69.84%   -0.99%     
==========================================
  Files         306      306              
  Lines       61725    61722       -3     
==========================================
- Hits        43725    43112     -613     
- Misses      18000    18610     +610     
Flag Coverage Δ
addons 92.88% <ø> (ø)
netcore 73.34% <100.00%> (-0.11%) ⬇️
netfx 68.03% <100.00%> (-1.43%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...nt/src/Microsoft/Data/Common/ActivityCorrelator.cs 100.00% <100.00%> (ø)

... and 25 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JRahnama JRahnama added this to the 5.2.0-preview1 milestone Apr 14, 2023
@DavoudEshtehari DavoudEshtehari added the 🐛 Bug! Issues that are bugs in the drivers we maintain. label Apr 14, 2023
@JRahnama
Copy link
Contributor

/azurepipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Kaur-Parminder Kaur-Parminder merged commit a6e7e0d into dotnet:main Apr 17, 2023
@cheenamalhotra cheenamalhotra deleted the fix-activity-correlator branch April 20, 2023 07:15
kant2002 pushed a commit to kant2002/SqlClient that referenced this pull request Jun 29, 2023
DavoudEshtehari added a commit to DavoudEshtehari/SqlClient that referenced this pull request Oct 25, 2023
DavoudEshtehari added a commit to DavoudEshtehari/SqlClient that referenced this pull request Oct 27, 2023
DavoudEshtehari added a commit to DavoudEshtehari/SqlClient that referenced this pull request Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug! Issues that are bugs in the drivers we maintain.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants