-
Notifications
You must be signed in to change notification settings - Fork 135
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
Serilog vendoring #512
Serilog vendoring #512
Conversation
c2e328e
to
9512835
Compare
5a3b946
to
3a6b1b5
Compare
@@ -24,6 +25,12 @@ internal Span(SpanContext context, DateTimeOffset? start) | |||
Context = context; | |||
ServiceName = context.ServiceName; | |||
StartTime = start ?? Context.TraceContext.UtcNow; | |||
|
|||
Log.Debug( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me, this is already going to be great information to have!
@@ -264,6 +270,17 @@ public void Finish(DateTimeOffset finishTimestamp) | |||
if (shouldCloseSpan) | |||
{ | |||
Context.TraceContext.CloseSpan(this); | |||
if (Log.IsEnabled(LogEventLevel.Debug)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this logging statement is hidden under the Log.IsEnabled
check whereas the span start isn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To prevent the interpolation. Happy to refactor it otherwise.
We have a bunch of spots in c++ that do this on that note.
src/vendors/Datadog.Trace.Vendoring/Datadog.Trace.Vendoring.csproj
Outdated
Show resolved
Hide resolved
src/vendors/Datadog.Trace.Vendoring/Datadog.Trace.Vendoring.csproj
Outdated
Show resolved
Hide resolved
src/vendors/Datadog.Trace.Vendoring/Datadog.Trace.Vendoring.csproj
Outdated
Show resolved
Hide resolved
src/vendors/Datadog.Trace.Vendoring/Datadog.Trace.Vendoring.csproj
Outdated
Show resolved
Hide resolved
src/vendors/Datadog.Trace.Vendoring/Datadog.Trace.Vendoring.csproj
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM aside from small changes to Datadog.Trace
project file.
… expected to update a new Datadog.Trace.nuspec file instead of Datadog.Trace.csproj, but that change was ultimately reverted in the final PR
* 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
* 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
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