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

Add exception information to traces #694

Closed
wants to merge 52 commits into from

Conversation

bachuv
Copy link
Collaborator

@bachuv bachuv commented Mar 16, 2022

This PR adds exception information to traces when an orchestration or activity throws an exception. Two tags are added to the trace Activity: otel.status_code and otel.status_description. These tags are how the Activity status is set.

activity?.SetTag("otel.status_code", "ERROR");
activity?.SetTag("otel.status_description", "description");

This is the approach we are using to set the Activity's status. This is what is listed in the .NET distributed tracing docs:

OpenTelemetry allows each Activity to report a Status that represents the pass/fail result of the work. .NET does not currently have a strongly typed API for this purpose but there is an established convention using Tags

Reference:
Add distributed tracing instrumentation - .NET | Microsoft Docs

Images:
image

image

image

pavisalavisa and others added 23 commits December 7, 2021 11:34
- Update unit tests
- Update C# version to 9.0
This should avoid the many `byte[]` allocations that happen periodically, and impact performance.

Co-authored-by: Itay Sagui <itsagui@microsoft.com>
This PR adds the SBOM manifest to the DurableTask.AzureStorage Release NuGet package. It adds a dotnet publish step and moves the manifest generator step to come before the dotnet pack step. It also adds a .nuspec file that specifies the additional files that need to get added to the NuGet package during dotnet pack.
Co-authored-by: Itay Sagui <itsagui@microsoft.com>
…e Fabric provider (#689)

* Adding an option to log structured events vis LoggerFactory in Service Fabric provider

* Bumping up the dll version
This PR fixes the license in the .nuspec files from MIT to Apache 2.0.
…g to newer release pipeline (#691)

* Adding SBOM to DT.AzureServiceFabric Release Nuget packages and moving to newer release pipeline

* Updating license text

* Updating AzureServiceFabric package generation to not include dependent binaries

* Removing publish step from the pipeline for AzureServiceFabric package

* Setting default platform for Service Fabric dependent binaries to x64 to get rid of build warnings. (SF binaries are x64 only)
@bachuv bachuv requested a review from cgillum March 16, 2022 23:53
@bachuv bachuv self-assigned this Mar 16, 2022
Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

I love this! I suggest we have a more specific value for otel.status_description.

src/DurableTask.Core/TaskActivityDispatcher.cs Outdated Show resolved Hide resolved
@cgillum
Copy link
Member

cgillum commented Mar 17, 2022

Tagging @jviau for comments.

@bachuv bachuv marked this pull request as ready for review March 17, 2022 19:06
bachuv and others added 14 commits April 13, 2022 10:54
* add multi instance query support logics

* have return type as OrchestrationStatusQueryResult

* remove unnecessary files copied from durable extension

* made property in OrchestrationQueryResult read only

* fix parameter name - make ContinuationToken nullable - throw exception if orchestrationState is null

* make continuationToken null in constructor
* Purge instance history

* Renamed Core/PurgeHistoryResult to Core/PurgeResult.cs to remove ambiguity

* Code refactor

* Minor style updates

Co-authored-by: Chris Gillum <cgillum@gmail.com>
* Also updating DT.Core to v2.9.0
* Also updating DT.AzureStorage to v1.11.0
Co-authored-by: amdeel <52223332+amdeel@users.noreply.github.com>
Co-authored-by: Itay Sagui <itsagui@microsoft.com>
* fix lease balancer logic for ownership leases

* exclude lease renewals from request throttling

* rename force to throttle as suggested

* fix missed occurrence of renaming throttle to force
* Also fixed app lease restart issue
- Fix misleading "lease lost" log message during shutdown
- Make lease release serial instead of parallel for better determinism
- Cleanup subscriptions during shutdown that were causing unexpected lease management operations
Base automatically changed from vabachu/reuseactivity to cgillum/opentelemetry May 16, 2022 23:27
@bachuv bachuv requested a review from cgillum August 24, 2022 18:46
@cgillum
Copy link
Member

cgillum commented Aug 24, 2022

It looks like this PR is suddenly showing changes from other PRs that have already been merged, so I'm having trouble seeing what I should actually be reviewing. @bachuv are you able to update the PR to show only your changes?

@bachuv
Copy link
Collaborator Author

bachuv commented Jan 18, 2023

Yes, I will update the PR to clean up the commit history. It's currently ahead of the branch cgillum/opentelemetry which is why it's showing additional changes. I will update that branch and then merge those changes. I'll do that this week and ping you when it's ready for review.

@bachuv
Copy link
Collaborator Author

bachuv commented Feb 7, 2023

Closing in favor of #853

@bachuv bachuv closed this Feb 7, 2023
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.