Skip to content

Standardization of Event Source tracing + Improvements for readability - #484

Merged
cheenamalhotra merged 14 commits into
dotnet:masterfrom
cheenamalhotra:sanitization
Mar 24, 2020
Merged

Standardization of Event Source tracing + Improvements for readability#484
cheenamalhotra merged 14 commits into
dotnet:masterfrom
cheenamalhotra:sanitization

Conversation

@cheenamalhotra

@cheenamalhotra cheenamalhotra commented Mar 23, 2020

Copy link
Copy Markdown
Member

Updates in EventSource tracing implementation:

  • Re-ordered Keywords
  • Code Organization for readability
  • Standardization of method names
  • Inline documentations
  • SNI Trace + Scope segregation
  • Event proofing
  • Adds support in ManualTests to enable tracing for all tests from pipeline.
  • Modifies enabling "UseManagedSNIOnWindows" from config.json instead of Env variable.

Also to mention:

  • This change also makes EventSource traces all disabled by default.
    End users would have to enable "ExecutionTrace" to continue to capture Begin/End execute commands.
  • Keeping this trace enabled by default does not seem appropriate when comparing with other EventSource implementations. As we head to 2.0 (major), we are making amendments to set things right.

# Conflicts:
#	src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
#	src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs
@cheenamalhotra cheenamalhotra added this to the 2.0.0-preview2 milestone Mar 23, 2020

@Wraith2 Wraith2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Other than the comments I noted it looks like just refactored names, so LGTM.

@cheenamalhotra

Copy link
Copy Markdown
Member Author

Enabling all traces seems to capture issues in Managed SNI tracing implementation from PR #461, to be continued tomorrow!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants