Fix | Revert Event source changes on TryBeginExecuteEvent and WriteEndExecuteEvent to address the failure on other MS products.

[JRahnama]

Other Microsoft products such as OpenTelemetry and Application Insight are facing difficulties in their products with the changes made to EventSource in v3.0.0.
This PR reverts those changes to accept:

1. ObjectId
2. DataSource
3. Database
4. CommandText

variables for TryBeginExecuteEvent and

1. ObjectId
2. CompositeState
3. SqlExceptionNumber

variables for TryEndExecuteEvent

@mbakalov can you check the changes from this build and confirm if the issue is resolved.

[Wraith2]

Interesting problem. Was it an issue because the connection id was added in the middle of the argument list or would any change at all cause issues?

It is unfortunate that we can't make the events richer. Can AppInsights and OpenTelemetry suggest ways that we can enhance the traces without breaking them?

[JRahnama]

Interesting problem. Was it an issue because the connection id was added in the middle of the argument list or would any change at all cause issues?

I do not have much detail on the problem, but apparently Expected payload for the BeginExecute is:

1. ObjectId
2. DataSource
3. Database
4. CommandText

And for EndExecute accordingly is:

1. ObjectId
2. CompositeState bitmask (0b001 -> successFlag, 0b010 -> isSqlExceptionFlag , 0b100 -> synchronousFlag)
3. SqlExceptionNumber

I think they are waiting those values in their own event listener.
I have tried to add the Message to the event attribute, but there were some limitation in the string setting, it should be constant and I was not able to add caller class name to the printed out message. I am still not sure if we could keep the message in there. If that is the case I can add a similar function for our use with a different name.

[Wraith2]

I've asked on the original issue in OpenTelemetry, hopefully we can get some useful feedback.

[mbakalov]

Cross-posting from the comment in the Otel issue: if you add additional payload at the end then it should be good enough to satisfy both OpenTelemetry and AppInsights. Hopefully this way works for everyone!

Happy to test out any changes to verify both OpenTelemetry and AppInsights.

[Wraith2]

Excellent, so we can just swap the field order around and it should be ok. The question is whether we want to do the same thing for all the events in SqlClient @JRahnama.

[JRahnama]

Excellent, so we can just swap the field order around and it should be ok. The question is whether we want to do the same thing for all the events in SqlClient

I will fix the part related to the recent request, I am not sure what you mean about all the events in
sqlClient. If you can elaborate a bit more I can start working on it if the team approves it.

[Wraith2]

I am not sure what you mean about all the events in sqlClient. If you can elaborate a bit more I can start working on it if the team approves it.

I thought there were other trace methods which took the connection id guid. I've looked and there don't seem to be, I was just mistaken.

Just changing BeginExecute to the version which has 4 payload items is enough, then the TryBeginExecute can call that one. Same pattern with EndExecute.

[Wraith2]

@mbakalov If you want to test the version from this PR the artifacts are at https://dev.azure.com/sqlclientdrivers-ci/7540d8a3-c1bc-4428-a007-d4b50b7762f4/_apis/build/builds/38263/artifacts?artifactName=Nuget&api-version=6.0&%24format=zip . There's a major version release coming up and I'm not sure if this can be pushed in at the last minute but if it can it'd be nice to get it done as soon as possible.

[mbakalov]

@Wraith2 - it would be nice to include this is possible!

Confirming that OpenTelemetry's tests are now passing with this new package version (they were failing on 3.0.0 before). I've not checked AppInsights yet, will try to do a bit later today.

cc @JRahnama

[mbakalov]

Confirming this fixes the Application Insights issue microsoft/ApplicationInsights-dotnet#2346 as well, verified with a simple web app: dependency is captured using sqlclient 2.1.3, then not captured using 3.0.0, then again captured using the ci nuget.

[DavoudEshtehari]

A freindly reminder: don't forget to add a complimentary note over this class and explain the
considerable criteria related to OpenTelemetry and ApplicationInsight.

[JRahnama]

A freindly reminder: don't forget to add a complimentary note over this class and explain the
considerable criteria related to OpenTelemetry and Application

There was an older EventSource version in the driver and it used to capture events for BeginExecute and EndExedute and nothing else. Most of the other teams were using that EventSource not the one recently added to the driver. A related comment is added to the Begin/End execute. Do you want me to add the comment to the entire class?

[DavoudEshtehari]

Yes, please. There is no guarantee it remains the same. So, it'd better have a general idea over the class with useful links for further changes to prevent any regression.