Skip to content

Standardization of Event Source tracing + Improvements for readability #484

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Mar 24, 2020
Merged

Standardization of Event Source tracing + Improvements for readability #484

merged 14 commits into from
Mar 24, 2020

Conversation

@cheenamalhotra
Copy link
Member

@cheenamalhotra cheenamalhotra commented Mar 23, 2020 •
edited
Loading

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.

@cheenamalhotra
Standardization of EventSource tracing + Improvements for readability
726bc5b
@cheenamalhotra
Merge branch 'master' into sanitization
ea4ece1 
# Conflicts:
#	src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
#	src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs
@cheenamalhotra
Capitalizations
301d001
@cheenamalhotra
unwanted refs
e08b7ee
@cheenamalhotra cheenamalhotra added this to the 2.0.0-preview2 milestone Mar 23, 2020
Wraith2
Wraith2 approved these changes Mar 24, 2020
View reviewed changes
Copy link
Contributor

@Wraith2 Wraith2 left a comment

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
Member Author

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

@cheenamalhotra cheenamalhotra merged commit 29b65c0 into dotnet:master Mar 24, 2020
@dependabot dependabot bot mentioned this pull request Jul 23, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@David-Engel David-Engel David-Engel approved these changes

+3 more reviewers

@karinazhou karinazhou karinazhou left review comments

@Wraith2 Wraith2 Wraith2 approved these changes

@JRahnama JRahnama JRahnama approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.0.0-preview2

Development

Successfully merging this pull request may close these issues.

5 participants

@cheenamalhotra @Wraith2 @David-Engel @karinazhou @JRahnama