Direct log submission by andrewlock · Pull Request #1945 · DataDog/dd-trace-dotnet

andrewlock · 2021-10-27T10:11:44Z

This PR adds support for directly shipping logs to the Datadog intake, so customers don't have to configure log tailing in the agent etc. Supports all our existing logs injection frameworks

Microsoft.Extensions.Logging.ILogger
Serilog
log4Net
NLog 2.1+ (This is more restrictive than for logs injection, but I don't think it's worth the effort to go earlier)

Each of the commits is pretty well self contained, so may be best to review sequentially.

Approach

This essentially takes the same approach as the official Datadog Serilog Sink. In various places I deviated from their approach however, as there were various opportunities for performance improvements (not serializing too JSON, then back to Dynamic, and back to JSON etc).

I partially vendored-in the Serilog batching sink provider, removed the Serilog-isms, and created an abstraction around that. Similar to other components, we have a singleton DatadogSink, (which implements the vendored batching sink) and is responsible for formatting and sending batches of logs. The LogsApi type takes care of the transport (similar to the Api class for traces). EDIT: much of the vendored code has been reworked

For serialization, I used a similar formatting to the CompactJsonFormatter, as that's supported in the logs documentation, serializing efficiently where possible. Without System.Text.Json's Utf8Writer, the best I could do to reduce memory impact was to reuse a stringbuilder, and have a persistent buffer for the UTF8 bytes. EDIT: Given System.Text.Json is built-in to netcore3.1+, I wonder if we should use that where available 🤔

We use automatic instrumentation to add the ILoggerProvider/SerilogSink as required, which then farms off the logs to the singleton Serilog Sink.

Current limitations

Currently only supports HTTP transport. ~~We could feasibly ship a PoC with that I think, but we may want to add TCP too. Shouldn't be too hard, but wasn't worth the effort right now I think.~~ TCP has been deprecated, so that solves that
The DirectLogSubmission instance is a singleton that's initialized with the profiler (as it requires some tracer settings etc, e.g. service names, disabled integrations). Much like runtime metrics, this cannot be reconfigured in code by creating a new Tracer, as that would cause significant additional complexity. Changed with 2.0 with the introduction of TracerManager
~~The batching options (batch size, buffer size etc) are currently hard coded but could easily be exposed as environment variables~~ At Colin's request, made these configurable too.
There are more optimisations we could make around serializing etc, but probably preferable to ship it.
I'm a tad concerned that we're just using the default timeouts everywhere. The LogsApi re-uses the same transports as the agent writer, but we're sending it over the public network, so there's a lot of failure points that could lead to timeouts being common. With the long timeouts we currently have, we could potentially end up buffering a lot of data. That's something we should probably address more generally though, rather than in this PR. EDIT: this has been partially addressed with the addition of the CircuitBreaker, though we should still reduce the HTTP timeouts IMO

Questions

The configuration key environment variable names are quite descriptive (i.e. long). Any preferences for alternative names?
Currently I'm grabbing the API Key directly from EnvironmentVariables only (as that's what AAS does too). Should we load it from multiple sources in the more typical way?
The default log intake URL is currently set to https://http-intake.logs.datadoghq.eu, as that's the same default used by the Datadog serilog sink. Do we want to keep that as the default, or should we use a different one by default? e.g. What is AAS's default? EDIT: updated to https://http-intake.logs.datadoghq.com:443, to match the default AAS DD_SITE setting

Testing

Added integration tests for each framework

Tested that enabling log shipping has no impact on logs injection
Can receive logs, and they have the TWoL tags added correctly (where expected)
Fixed some bugs in the NLog logs injection tests (unrelated to log shipping)

Also did manual testing of each framework, and confirmed I can see the logs in the UI:

@DataDog/apm-dotnet

colin-higgins · 2021-10-27T13:58:51Z

        }

-        public async Task<IApiResponse> PostAsync(ArraySegment<byte> traces)
+        public async Task<IApiResponse> PostAsync(ArraySegment<byte> traces, string contentType)


Should we rename the traces parameter now that this isn't just traces?

done in f4992c2

colin-higgins · 2021-10-27T14:00:24Z

                await memoryStream.FlushAsync().ConfigureAwait(false);
                memoryStream.Seek(0, SeekOrigin.Begin);
                var buffer = memoryStream.GetBuffer();
-                _headers.Add("Content-Type", "application/json");


I assume this was removed because it was redundant? and conflicted with the now dynamic contentType?
Edit: Oh I see it being set was moved to the PostSegmentAsync call.

pierotibou

I'm halfway across the review , I'll finish later.
Main concern, I don't think listing TCP is a good idea if we target datadlog logs api (and it would be better to target the new API from the beginning)

pierotibou · 2021-11-19T09:17:14Z

+            /// <summary>
+            /// Configuration key for the transport to use for communicating with the trace agent.
+            /// Default value is <c>HTTP</c>.
+            /// Options available: <c>HTTP</c>,<c>TCP</c>


Where are the logs send? On the Logs side they have deprecated TCP, but maybe it makes sense in that case

On the Logs side they have deprecated TCP

Oh, didn't realise that! I haven't actually added support for TCP yet anyway, so probably just best to remove all references to it.

Removed the TCP option in 496600b

pierotibou · 2021-11-19T09:21:19Z

+            DirectLogSubmissionHost = source?.GetString(ConfigurationKeys.DirectLogSubmission.Host)
+                ?? HostMetadata.GetHost();
+            DirectLogSubmissionSource = source?.GetString(ConfigurationKeys.DirectLogSubmission.Source)
+                                     ?? "csharp"; // default


[Nit] Out of curiosity, could be any language in .net, so why using "csharp" as a default?

Anecdotally, csharp is about 99% of the market, so I think it makes sense 😉 It also mirrors the serilog sink behaviour that this was based on

It's kind of a strange thing to even differentiate between the dotnet languages at this level as they can all intercall into eachother anyways... Like this code would have been from c#... client code could be any mix...
Is there any sort of precedent? Maybe in java? As they could have any number of JVM languages too.

I totally agree in principal, I'm basing this on the logs documentation which describes the source as needing to be csharp https://docs.datadoghq.com/logs/log_collection/csharp/?tab=serilog#configure-your-datadog-agent I put it down to legacy choices way back!

pierotibou

That's a long PR :)

colin-higgins · 2021-11-30T14:45:56Z

+    /// including the desire to send batches "when full" rather than continuing to buffer, and so-on.
+    /// </remarks>
+    internal class BatchedConnectionStatus
+    {


I don't think I am comfortable with the scale of the exponential backoff here.
We could be piling up for several minutes it seems like.
Minimally I think these retries should be capped to something like 4, and at best case they should be configurable.

Yeah, 10 minutes does seem like a long time. That said, if there's no connection to be made, what's the best option? We have a size limit on the queue size, so we're not going to blow up indefinitely, and arguably those first logs when things started to go wrong might be more important than subsequent ones, so maybe better to hang onto those and drop new logs? Not sure what's best tbh, though agree configurable is a good way we can blame it on the user when it's wrong 😉

Actually, looking through this again, there's significant overlap with the BatchedConnectionStatus and the LogsApi. Both are doing retries/exponential backoff, which we definitely don't want. That said, we're never throwing in DatadogSink.EmitBatch(), so none of the retry code in BatchedConnectionStatus will ever run

I'm inclined to do the following:

Drop BatchedConnectionStatus entirely

The LogsApi has better knowledge about when a batch should be dropped etc, so I think SendLogsAsync should return a result with what to do (Success/DropBatch/DropQueue). That can bubble up via EmitBatch() so we can remove BatchedConnectionStatus.

Alternatively, rip the whole thing out and do our own version 😬 The Serilog API is cleaner in some respects, as it separates the logic for when to retry etc from the implementors, but it's using exceptions for flow control, and we're handling that ourselves anyway.

What do you think?

Rip out BatchedConnectionStatus only?

Tear it all down?

Do 1. or 2. in a follow-up PR

I'm okay with 1 coming out, do you think we should remove it now?

Yeah, I can tear it out as part of this PR. I guess it just adds confusion for other people looking at it now anyway 👍

Reworked the vast majority of the batching implementation in 2dfc44e.

Dropped BatchedConnectionStatus

Dropped PeriodicTimer

Added CircuitBreaker

Removed some of the complexity around starting/stopping the sink with a tcs. We could optimise it more, but seems sufficient for now to me.

In the previous version, everything was added to a giant (persistent) StringBuilder, we then call ToString(), and convert the string to a byte array (which is then thrown away). The string and utf8 array are ultimately garbage. In this version, we keep a smaller persistent StringBuilder (one for each log, instead of all logs), convert that to a string, and write the bytes to a persistent byte array. Only the intermediate string is garbage. The benefit should be that we have less chance of the string/byte array ending up on the LOH, and we can reuse the UTF8 array

…bled These are constants, and so really doesn't make sense to not be adding these automatically. Arguably, we could also be injecting span_id and trace_id, but that's probably something to think about if we end up replacing the logs injection in general.

It's not implemented anyway, and is now deprecated

As we don't just use it for sending traces any more

In case there are any issues loading the assemblies etc

Set the defaults just in tracer settings, to avoid conflicting values later

…egration. Shouldn't happen, but log a message, and write null instead.

Adding a circuit breaker, plus removing what feels like quite a lot of complexity

Helpful in upcoming changes, and why not

- Don't control log shipping via the "global" IntegrationIds. Having two ways to enable/disable the integration is confusing. Plus this change simplifies things, as we know the configuration at startup time. - Remove warning when logs injection disabled. We automatically add the values anyway, so doesn't matter - Cache the direct log submission settings and sink in the targets/appenders, seeing as none of the settings they use are currently configurable in code - Enabled some nullable ref types (as some things, e.g. the `LogFormatter` in ILogger, _may_ be null, so adds some sanity checks) - Add some try-catches around static initialization for belt-and-braces

We support NLog Logs injection from version 1.0.0.505, but currently only support direct log shipping from 2.1.0. This bumps the "default" package version to be 2.1.0 in the nlog tests

Recent benchmarking shows that we should prefer interfaces generally, so converted to that approach instead. Also tidied up some things, like removing unused properties, fixing namespaces etc.

The formatter test was actually testing Serilog, and it also wouldn't work correctly in some cases AFAICT.

andrewlock · 2021-12-23T16:10:45Z

Code Coverage Report 📊

✔️ Merging #1945 into master will not change line coverage
⚠️ Merging #1945 into master will will decrease branch coverage by 1%
⛔ Merging #1945 into master will will increase complexity by 719

	master	#1945	Change
Lines	`8963` / `12180`	`9851` / `13442`
Lines %	`74%`	`73%`	`0%` ✔️
Branches	`4093` / `5911`	`4478` / `6557`
Branches %	`69%`	`68%`	`-1%` ⚠️
Complexity	`7095`	`7814`	`719` ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#1945	Change
Lines %	`74%`	`73%`	`0%` ✔️
Branches %	`69%`	`68%`	`-1%` ⚠️
Complexity	`7095`	`7814`	`719` ⛔

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.ClrProfiler.AutoInstrumentation.Couchbase.IIOServiceExecuteIntegrationBis	`-100%` ⛔	`0%` ✔️	`0` ✔️
Datadog.Trace.ClrProfiler.AutoInstrumentation.Testing.MsTestV2.TestMethodRunnerExecuteIntegration	`-33%` ⛔	`-19%` ⛔	`0` ✔️
Datadog.Trace.Ci.Agent.CIAgentWriter	`-10%` ⛔	`-21%` ⛔	`0` ✔️
Datadog.Trace.ClrProfiler.AutoInstrumentation.Testing.NUnit.NUnitCompositeWorkItemSkipChildrenIntegration	`-8%` ⛔	`-8%` ⛔	`0` ✔️
Datadog.Trace.TracerManager	`-1%` ⚠️	`-6%` ⛔	`6` ⛔
Datadog.Trace.PlatformHelpers.HostMetadata	`6%` ✔️	`0%` ✔️	`0` ✔️
[Datadog.Trace.ClrProfiler.CallTarget.Handlers.BeginMethodHandler`5](https://github.com/DataDog/dd-trace-dotnet/tree/4fee0f22f3b79f5f239e7288b3d854f47ae1cb89/tracer/src/Datadog.Trace/ClrProfiler/CallTarget/Handlers/BeginMethodHandler`3.cs)	`8%` ✔️	`10%` ✔️	`0` ✔️

The following classes were added in #1945:

File	Line coverage	Branch coverage	Complexity
Datadog.Trace.ClrProfiler.AutoInstrumentation.Log4Net.AppenderAttachedImplIntegration	`100%`	`81%`	`17`
Datadog.Trace.ClrProfiler.AutoInstrumentation.Logging.ILogger.DatadogLoggingScope	`76%`	`47%`	`35`
Datadog.Trace.ClrProfiler.AutoInstrumentation.Logging.ILogger.DirectSubmission.DirectSubmissionLogger	`96%`	`88%`	`12`
Datadog.Trace.ClrProfiler.AutoInstrumentation.Logging.ILogger.DirectSubmission.DirectSubmissionLoggerProvider	`75%`	`100%`	`6`
Datadog.Trace.ClrProfiler.AutoInstrumentation.Logging.ILogger.DirectSubmission.Formatting.LogEntry`1	`100%`	`100%`	`1`
...And 40 more

5 classes were removed from Datadog.Trace in #1945

View the full reports for further details:

andrewlock · 2021-12-23T21:45:11Z

Benchmarks Report 🐌

Benchmarks for #1945 compared to master:

1 benchmarks are faster, with geometric mean 1.176
1 benchmarks have fewer allocations
1 benchmarks have more allocations

The following thresholds were used for comparing the benchmark speeds:

Mann–Whitney U test with statistical test for significance of 5%
Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Fewer allocations 🎉

Fewer allocations 🎉 in #1945

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.AgentWriterBenchmark.WriteAndFlushEnrichedTraces‑net472	3.17 KB	3.16 KB	-3 B	-0.09%

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Allocated
master	`WriteAndFlushEnrichedTraces`	net472	998μs	2.28μs	8.54μs	3.17 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	785μs	1.7μs	6.57μs	2.57 KB
#1945	`WriteAndFlushEnrichedTraces`	net472	983μs	3.25μs	12.6μs	3.16 KB
#1945	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	774μs	1.97μs	7.39μs	2.57 KB

Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
master	`SendRequest`	netcoreapp3.1	304μs	1.61μs	7.87μs	0.147	19.59 KB
#1945	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#1945	`SendRequest`	netcoreapp3.1	310μs	1.26μs	5.03μs	0.15	19.59 KB

Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteNonQuery`	net472	1.77μs	8.66ns	35.7ns	0.0904	0.000895	570 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.61μs	9.33ns	84.5ns	0.00858	0	608 B
#1945	`ExecuteNonQuery`	net472	1.76μs	8.96ns	52.2ns	0.0901	0.000867	570 B
#1945	`ExecuteNonQuery`	netcoreapp3.1	1.7μs	7.58ns	29.4ns	0.00857	0	608 B

Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net472	2.65μs	13ns	55.2ns	0.121	778 B
master	`CallElasticsearch`	netcoreapp3.1	1.68μs	6.55ns	24.5ns	0.0103	768 B
master	`CallElasticsearchAsync`	net472	3.31μs	17.5ns	90.9ns	0.142	915 B
master	`CallElasticsearchAsync`	netcoreapp3.1	1.84μs	8.37ns	31.3ns	0.0126	888 B
#1945	`CallElasticsearch`	net472	2.71μs	13.8ns	91.9ns	0.121	778 B
#1945	`CallElasticsearch`	netcoreapp3.1	1.7μs	9.19ns	49.5ns	0.0102	768 B
#1945	`CallElasticsearchAsync`	net472	3.32μs	16.9ns	75.8ns	0.142	915 B
#1945	`CallElasticsearchAsync`	netcoreapp3.1	1.88μs	7.64ns	27.5ns	0.0118	888 B

Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net472	3.19μs	15.7ns	64.7ns	0.162	1.04 KB
master	`ExecuteAsync`	netcoreapp3.1	1.97μs	8.13ns	29.3ns	0.0145	1.01 KB
#1945	`ExecuteAsync`	net472	3.25μs	12.2ns	47.1ns	0.161	1.04 KB
#1945	`ExecuteAsync`	netcoreapp3.1	2.07μs	6.8ns	26.3ns	0.0135	1.01 KB

Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	6.87μs	37.5ns	209ns	0.351	2.24 KB
master	`SendAsync`	netcoreapp3.1	4.57μs	18.1ns	70ns	0.0295	2.14 KB
#1945	`SendAsync`	net472	6.84μs	31.4ns	122ns	0.35	2.24 KB
#1945	`SendAsync`	netcoreapp3.1	4.51μs	6.33ns	22.8ns	0.0297	2.14 KB

Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	4.15μs	18.9ns	70.8ns	0.22	1.41 KB
master	`EnrichedLog`	netcoreapp3.1	3.31μs	15.3ns	59.2ns	0.0213	1.49 KB
#1945	`EnrichedLog`	net472	4.31μs	15.2ns	56.7ns	0.221	1.41 KB
#1945	`EnrichedLog`	netcoreapp3.1	3.37μs	9.94ns	37.2ns	0.0198	1.49 KB

Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #1945

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.Log4netBenchmark.EnrichedLog‑netcoreapp3.1	4.17 KB	4.17 KB	1 B	0.02%

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	179μs	464ns	1.74μs	0.529	0.176	4.29 KB
master	`EnrichedLog`	netcoreapp3.1	160μs	734ns	3.44μs	0	0	4.17 KB
#1945	`EnrichedLog`	net472	180μs	472ns	1.7μs	0.457	0.183	4.29 KB
#1945	`EnrichedLog`	netcoreapp3.1	156μs	491ns	1.9μs	0	0	4.17 KB

Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	8.52μs	30.3ns	117ns	0.499	3.19 KB
master	`EnrichedLog`	netcoreapp3.1	7.1μs	38.3ns	213ns	0.0479	3.56 KB
#1945	`EnrichedLog`	net472	8.38μs	29.4ns	114ns	0.497	3.19 KB
#1945	`EnrichedLog`	netcoreapp3.1	7.03μs	39.5ns	271ns	0.0502	3.56 KB

Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net472	2.59μs	14ns	81.8ns	0.154	987 B
master	`SendReceive`	netcoreapp3.1	2.09μs	7.27ns	34.1ns	0.0137	984 B
#1945	`SendReceive`	net472	2.51μs	8.79ns	34ns	0.154	987 B
#1945	`SendReceive`	netcoreapp3.1	2.13μs	10.7ns	50.4ns	0.0138	984 B

Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	6.89μs	33.4ns	138ns	0.285	1.83 KB
master	`EnrichedLog`	netcoreapp3.1	5.77μs	18.5ns	71.5ns	0.02	1.45 KB
#1945	`EnrichedLog`	net472	7.05μs	33.5ns	134ns	0.283	1.83 KB
#1945	`EnrichedLog`	netcoreapp3.1	5.76μs	6.34ns	22.8ns	0.02	1.45 KB

Benchmarks.Trace.SpanBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #1945

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net472	1.176	1,483.56	1,261.07

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net472	1.01μs	4.98ns	22.3ns	0.0677	433 B
master	`StartFinishSpan`	netcoreapp3.1	943ns	3.63ns	13.1ns	0.00607	432 B
master	`StartFinishScope`	net472	1.49μs	7.6ns	35.7ns	0.08	514 B
master	`StartFinishScope`	netcoreapp3.1	1.11μs	5.12ns	19.8ns	0.00787	552 B
#1945	`StartFinishSpan`	net472	1.03μs	3.27ns	12.7ns	0.0679	433 B
#1945	`StartFinishSpan`	netcoreapp3.1	977ns	2.64ns	10.2ns	0.00591	432 B
#1945	`StartFinishScope`	net472	1.25μs	4.94ns	18.5ns	0.0804	514 B
#1945	`StartFinishScope`	netcoreapp3.1	1.14μs	3.12ns	12.1ns	0.00795	552 B

andrewlock · 2021-12-24T16:05:06Z

Closing in favour of #2240, which is just a reworking of the same content to try and make it easier to review

andrewlock added type:new-feature area:automatic-instrumentation Automatic instrumentation managed C# code (Datadog.Trace.ClrProfiler.Managed) needs-docs-update area:logs-injection labels Oct 27, 2021

andrewlock changed the title ~~Andrew/direct log submission~~ Direct log submission Oct 27, 2021

This comment has been minimized.

Sign in to view

andrewlock force-pushed the andrew/direct-log-submission branch from e380805 to 87409ee Compare October 27, 2021 13:31

andrewlock marked this pull request as ready for review October 27, 2021 13:34

andrewlock requested a review from a team as a code owner October 27, 2021 13:34

andrewlock mentioned this pull request Oct 27, 2021

Direct log submission PoC #1846

Closed

colin-higgins reviewed Oct 27, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

pierotibou requested changes Nov 19, 2021

View reviewed changes

andrewlock force-pushed the andrew/direct-log-submission branch from 87409ee to d56d2bc Compare November 24, 2021 13:44

andrewlock requested a review from a team as a code owner November 24, 2021 13:44

andrewlock force-pushed the andrew/direct-log-submission branch from d56d2bc to 0fdde18 Compare November 24, 2021 14:44

This comment has been minimized.

Sign in to view

pierotibou reviewed Nov 26, 2021

View reviewed changes

andrewlock mentioned this pull request Nov 26, 2021

Re-implement NLog logs injection #2096

Merged

This comment has been minimized.

Sign in to view

colin-higgins reviewed Nov 29, 2021

View reviewed changes

Comment thread ...ace/ClrProfiler/AutoInstrumentation/Logging/Log4Net/DirectSubmission/LoggingEventDuckBase.cs Outdated

colin-higgins reviewed Nov 29, 2021

View reviewed changes

Comment thread .../ClrProfiler/AutoInstrumentation/Logging/NLog/DirectSubmission/DirectSubmissionNLogTarget.cs

colin-higgins reviewed Nov 30, 2021

View reviewed changes

Comment thread tracer/src/Datadog.Trace/Logging/DirectSubmission/Sink/PeriodicBatching/BatchingSink.cs Outdated

andrewlock force-pushed the andrew/direct-log-submission branch from 1201ddb to f4992c2 Compare November 30, 2021 14:27

colin-higgins reviewed Nov 30, 2021

View reviewed changes

andrewlock added 20 commits December 23, 2021 15:05

Remove TCP as an option

daccfa4

It's not implemented anyway, and is now deprecated

Improve documentation for GlobalTags

9b59e59

Add unit tests for DirectSubmissionLogLevelExtensions

dd0cba6

Replace v1 Logs API with the v2 API

3fb1f7b

Rename traces -> bytes in IApiRequest

1743b0d

As we don't just use it for sending traces any more

Don't throw for unknown log levels - return "UNKNOWN" instead

965f5d0

Remove comment

dcb6385

Add try-catch to LoggerFactoryIntegrationCommon helper

9d284a9

In case there are any issues loading the assemblies etc

Allow setting batching options for the logs sink from configuration keys

b8c7347

Set the defaults just in tracer settings, to avoid conflicting values later

Don't throw if we get an unknown LogEventPropertyValue in Serilog int…

489f405

…egration. Shouldn't happen, but log a message, and write null instead.

We don't support net45 any more 🎉

7e1f787

Replace the bulk of the vendored Serilog periodic BatchingSink

59c29e7

Adding a circuit breaker, plus removing what feels like quite a lot of complexity

Enable nullable ref types in direct submission code

9f441ce

Helpful in upcoming changes, and why not

Update NLog injection tests to use different NLog version by default

caf1911

We support NLog Logs injection from version 1.0.0.505, but currently only support direct log shipping from 2.1.0. This bumps the "default" package version to be 2.1.0 in the nlog tests

Update all proxies to use best practices

596b124

Recent benchmarking shows that we should prefer interfaces generally, so converted to that approach instead. Also tidied up some things, like removing unused properties, fixing namespaces etc.

Update integration definitions with new namespace

a2ebb3f

Fix Log4Net formatting

64ef062

The formatter test was actually testing Serilog, and it also wouldn't work correctly in some cases AFAICT.

andrewlock force-pushed the andrew/direct-log-submission branch from bad556c to 64ef062 Compare December 23, 2021 15:06

This was referenced Dec 24, 2021

Explicitly state contentType when calling IApiRequest.PostAsync() #2237

Merged

Move logs-injection related classes to sub-folder #2238

Merged

Direct log submission #2240

Merged

andrewlock closed this Dec 24, 2021

andrewlock deleted the andrew/direct-log-submission branch February 16, 2022 09:59

andrewlock added the area:direct-log-submission label Mar 16, 2023

Conversation

andrewlock commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Approach

Current limitations

Questions

Testing

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colin-higgins Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

pierotibou left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewlock Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colin-higgins Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

pierotibou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewlock commented Dec 23, 2021

Code Coverage Report 📊

Datadog.Trace Breakdown ✔️

Uh oh!

andrewlock commented Dec 23, 2021

Benchmarks Report 🐌

Benchmark details

Fewer allocations 🎉 in #1945

andrewlock commented Oct 27, 2021 •

edited

Loading

colin-higgins Oct 27, 2021 •

edited

Loading

pierotibou left a comment •

edited

Loading

andrewlock Nov 19, 2021 •

edited

Loading

colin-higgins Nov 29, 2021 •

edited

Loading