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

Diagnostics: Add synchronization context Part 1 #1587

Merged

Conversation

j82w
Copy link
Contributor

@j82w j82w commented Jun 2, 2020

Pull Request Template

Description

This is part 1 with mostly just the diagnostic context changes and the CosmosClient updated to use the new interface. Other classes will be done in follow up PRs.

  1. Diagnostic context is now generated at the top level to ensure the correct total time is always captured.
  2. The synchronization context delay is now being tracked
  3. Added operation name to the diagnostics

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Closing issues

Put closes #XXXX in your comment to auto-close the issue that your PR fixes (if such).

Assignee

Please add yourself as the assignee

Projects

Please add relevant projects so this issue can be properly tracked.

@j82w j82w added the Diagnostics Issues around diagnostics and troubleshooting label Jun 2, 2020
@j82w j82w self-assigned this Jun 2, 2020
@kirankumarkolli
Copy link
Member

kirankumarkolli commented Jun 2, 2020

    internal override async Task<ContainerProperties> GetCachedContainerPropertiesAsync(

Unrelated to this PR:

Container has its own implementation for it.
Also it might be in critial path if leveraged across, won't context creation impact perf?


Refers to: Microsoft.Azure.Cosmos/src/Resource/ClientContextCore.cs:318 in b8bc976. [](commit_id = b8bc976, deletion_comment = False)

@@ -309,7 +309,7 @@ public virtual Task<AccountProperties> ReadAccountAsync()
/// <param name="id">The Cosmos database id</param>
/// <remarks>
/// <see cref="Database"/> proxy reference doesn't guarantee existence.
/// Please ensure database exists through <see cref="CosmosClient.CreateDatabaseAsync(DatabaseProperties, int?, RequestOptions, CancellationToken)"/>
/// Please ensure database exists through <see cref="CosmosClient.CreateDatabaseAsync(string, int?, RequestOptions, CancellationToken)"/>
Copy link
Member

Choose a reason for hiding this comment

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

Are there any mechanisms which we could use to get consistency?
Like referring real code snippet like in the public docs?

Non blocking but will immensely help forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was pointing to a real snippet of code, it's just an internal method.

@j82w j82w merged commit 1785ee0 into master Jun 16, 2020
@j82w j82w deleted the users/jawilley/diagnostics/includeTaskRunTime_newContracts branch June 16, 2020 11:23
kirankumarkolli pushed a commit that referenced this pull request Jul 11, 2020
* Diagnostic refactor for synchronization context

* Added Database changes to give more context on how it will look for inline classes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Diagnostics Issues around diagnostics and troubleshooting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants