Serilog vendoring#512
Merged
Merged
Conversation
c2e328e to
9512835
Compare
colin-higgins
commented
Sep 25, 2019
5a3b946 to
3a6b1b5
Compare
| ServiceName = context.ServiceName; | ||
| StartTime = start ?? Context.TraceContext.UtcNow; | ||
|
|
||
| Log.Debug( |
Contributor
There was a problem hiding this comment.
I was actually about to comment here that I wanted to add logging about the traceId and spanId. Regardless of the user's LOGS_INJECTION_ENABLED setting, I was thinking we should always be adding the traceId and spanId with our own managed logging. Do you think we should use the event subscriber approach or do you think this beginning/end logging is sufficient?
Member
Author
There was a problem hiding this comment.
I think beginning and end logging is sufficient. I don't want to add TOO much to the context, as that'll be logged with everything. Maybe if we start with this and decide it's not enough we can rethink it?
Contributor
There was a problem hiding this comment.
Sounds good to me, this is already going to be great information to have!
| if (shouldCloseSpan) | ||
| { | ||
| Context.TraceContext.CloseSpan(this); | ||
| if (Log.IsEnabled(LogEventLevel.Debug)) |
Contributor
There was a problem hiding this comment.
Is there a reason this logging statement is hidden under the Log.IsEnabled check whereas the span start isn't?
Member
Author
There was a problem hiding this comment.
To prevent the interpolation. Happy to refactor it otherwise.
We have a bunch of spots in c++ that do this on that note.
lucaspimentel
requested changes
Oct 11, 2019
lucaspimentel
left a comment
Member
There was a problem hiding this comment.
LGTM aside from small changes to Datadog.Trace project file.
zacharycmontoya
approved these changes
Oct 11, 2019
lucaspimentel
approved these changes
Oct 11, 2019
zacharycmontoya
added a commit
that referenced
this pull request
Oct 17, 2019
… expected to update a new Datadog.Trace.nuspec file instead of Datadog.Trace.csproj, but that change was ultimately reverted in the final PR
Merged
zacharycmontoya
added a commit
that referenced
this pull request
Oct 17, 2019
* Revert changes to the SynchronizeVersions tool made in #512, where it expected to update a new Datadog.Trace.nuspec file instead of Datadog.Trace.csproj, but that change was ultimately reverted in the final PR * Update product version from 1.7.0 to 1.8.0
macrogreg
pushed a commit
that referenced
this pull request
Aug 20, 2021
* Revert changes to the SynchronizeVersions tool made in #512, where it expected to update a new Datadog.Trace.nuspec file instead of Datadog.Trace.csproj, but that change was ultimately reverted in the final PR * Update product version from 1.7.0 to 1.8.0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes proposed in this pull request:
Strong sign and reference vendors projectUse nuspec file to deploy vendor library with Datadog.TraceUpdate WIX files to include new dependenciesUpdate CI to build nuget package based on NuSpec@DataDog/apm-dotnet