[Exporter.Geneva] Add TLDTraceExporter #662

utpilla · 2022-09-27T02:46:19Z

Changes

Add TLDTraceExporter
GenevaTraceExporter to use TLDExporter when the user provides "PrivatePreviewEnableTraceLoggingDynamic=true" in the connectionString

TODOs:

Use a single ThreadLocal instead of multiple
Avoid potential unbounded memory usage for List<KeyValuePair<string, object>> keyValuePairs

Setup and usage:

using var tracerProvider = Sdk.CreateTracerProviderBuilder()
            .SetSampler(new AlwaysOnSampler())
            .AddSource(Source.Name)
            .AddGenevaTraceExporter(options =>
            {
                options.ConnectionString = "EtwSession=OpenTelemetry;PrivatePreviewEnableTraceLoggingDynamic=true";// Add PrivatePreviewEnableTraceLoggingDynamic=true in the connection string
                options.PrepopulatedFields = new Dictionary<string, object>
                {
                    ["cloud.role"] = "BusyWorker",
                    ["cloud.roleInstance"] = "CY1SCH030021417",
                    ["cloud.roleVer"] = "9.0.15289.2",
                };
            })
            .Build();

//Emit activity
using (var activity = Source.StartActivity("SayHello"))
        {
            activity?.SetTag("foo", 1);
            activity?.SetTag("bar", "Hello, World!");
            activity?.SetTag("baz", new int[] { 1, 2, 3 });
            activity?.SetStatus(ActivityStatusCode.Ok);
        }

You could us any ETW collection tools such as WPA, PerfView etc. to view the data:

Example with PerfView:
For the connectionString = "EtwSession=OpenTelemetry;PrivatePreviewEnableTraceLoggingDynamic=true", you can run the following command in Command Prompt:

PerfView.exe collect -MaxCollectSec:3600 -NoGui /onlyProviders=*OpenTelemetry

codecov · 2022-09-27T03:00:35Z

Codecov Report

Merging #662 (4d7e856) into main (70f9f0f) will decrease coverage by 8.29%.
The diff coverage is 29.17%.

❗ Current head 4d7e856 differs from pull request most recent head a4168b7. Consider uploading reports for the commit a4168b7 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #662      +/-   ##
==========================================
- Coverage   79.07%   70.77%   -8.30%     
==========================================
  Files         182      187       +5     
  Lines        5614     6724    +1110     
==========================================
+ Hits         4439     4759     +320     
- Misses       1175     1965     +790

Impacted Files	Coverage Δ
...orter.Geneva/TLDExporter/UncheckedASCIIEncoding.cs	`15.68% <15.68%> (ø)`
...ry.Exporter.Geneva/External/TraceLoggingDynamic.cs	`22.46% <22.46%> (ø)`
...lemetry.Exporter.Geneva/TLDExporter/TLDExporter.cs	`27.84% <27.84%> (ø)`
...ry.Exporter.Geneva/TLDExporter/TLDTraceExporter.cs	`32.51% <32.51%> (ø)`
...xporter.Geneva/Internal/ConnectionStringBuilder.cs	`93.58% <60.00%> (-2.31%)`	⬇️
...etry.Exporter.Geneva/TLDExporter/JsonSerializer.cs	`61.26% <61.26%> (ø)`
...enTelemetry.Exporter.Geneva/GenevaTraceExporter.cs	`75.00% <75.00%> (-5.00%)`	⬇️
...ter.Geneva/MsgPackExporter/MsgPackTraceExporter.cs	`92.42% <0.00%> (-0.51%)`	⬇️

utpilla · 2022-09-27T20:08:26Z

src/OpenTelemetry.Exporter.Geneva/TLDExporter/TLDTraceExporter.cs

+    private readonly EventProvider eventProvider;
+    private static readonly ThreadLocal<EventBuilder> eventBuilder = new(() => new(UncheckedASCIIEncoding.SharedInstance));
+    private static readonly ThreadLocal<List<KeyValuePair<string, object>>> keyValuePairs = new(() => new());
+    private static readonly ThreadLocal<KeyValuePair<string, object>[]> partCFields = new(() => new KeyValuePair<string, object>[120]); // This is used to temporarily store the PartC fields from tags


This might lead to IndexOutOfRange exceptions if there are too many Part C tags. I chose the size as 120 as we cannot exceed 127 fields otherwise the event gets dropped and we have at least around 7 fields from Part A and Part B itself.

utpilla · 2022-09-27T20:10:34Z

src/OpenTelemetry.Exporter.Geneva/TLDExporter/TLDTraceExporter.cs

+
+    private readonly EventProvider eventProvider;
+    private static readonly ThreadLocal<EventBuilder> eventBuilder = new(() => new(UncheckedASCIIEncoding.SharedInstance));
+    private static readonly ThreadLocal<List<KeyValuePair<string, object>>> keyValuePairs = new(() => new());


This is used for serializing ActivityLinks and env_properties. I chose a List here instead of an array as we don't know how many such fields there could be.

What's the initial capacity? (and what's the reallocation behavior? e.g. would this give OOM?)

What's the initial capacity?

I let it use the default behavior.

(and what's the reallocation behavior? e.g. would this give OOM?)

I understand that this might lead to unbounded memory usage and possibly even lead to OOM in extreme cases but I didn't know if we could enforce some sort of limitation on the number of links or env_properties allowed. It's straightforward for PartC fields as the TLD spec enforces a limit on the number of fields allowed. In this case though, links or env_properties would only count towards one field.

Could we enforce such a limitation? If yes, we could very well use an array and just tell the user that they have too many links or env_properties if they exceed a certain number. If not, I guess we would have to go back to the two iterations approach.

Added a TODO for this.

utpilla · 2022-09-27T20:12:56Z

src/OpenTelemetry.Exporter.Geneva/TLDExporter/TraceLoggingDynamic.cs

+#pragma warning disable CA5350
+#pragma warning disable CA5392
+
+internal sealed class EventProvider


I have simply copied the contents of this file here: https://github.com/microsoft/tracelogging/blob/main/etw/cs/traceloggingdynamic/TraceLoggingDynamic.cs

Only additional changes that I made are that I have marked the types internal and internal sealed.

.editorconfig

reyang · 2022-09-27T20:18:17Z

src/OpenTelemetry.Exporter.Geneva/TLDExporter/TLDTraceExporter.cs

+    private static readonly string INVALID_SPAN_ID = default(ActivitySpanId).ToHexString();
+
+    private readonly EventProvider eventProvider;
+    private static readonly ThreadLocal<EventBuilder> eventBuilder = new(() => new(UncheckedASCIIEncoding.SharedInstance));


Which one would be better?

Having 3 ThreadLocals

Having a single ThreadLocal which points to a complex struct/object with 3 fields

I have no strong preference. I suppose there's no performance implications of using one over the other. If the second approach looks more readable, I could make that change.

Added a TODO for this.

…xporter

github-actions · 2022-10-05T03:44:13Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

github-actions · 2022-10-18T04:02:40Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

…xporter

github-actions · 2022-10-28T03:46:40Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

github-actions · 2022-11-05T03:33:59Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

…xporter

Add TLDTraceExporter

308b1f9

utpilla requested a review from a team September 27, 2022 02:46

utpilla added the comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva label Sep 27, 2022

github-actions bot assigned cijothomas, CodeBlanch, mic-max, reyang, utpilla and Yun-Ting Sep 27, 2022

github-actions bot requested review from cijothomas, CodeBlanch, mic-max, reyang and Yun-Ting September 27, 2022 02:46

utpilla added 2 commits September 26, 2022 19:49

Fix formatting

ce52324

Fix format

e521601

Update TLDExporter code

1504d17

utpilla commented Sep 27, 2022

View reviewed changes

reyang reviewed Sep 27, 2022

View reviewed changes

.editorconfig Outdated Show resolved Hide resolved

reyang reviewed Sep 27, 2022

View reviewed changes

utpilla added 2 commits September 27, 2022 13:33

Merge remote-tracking branch 'origin/main' into utpilla/Add-TLDTraceE…

114a40e

…xporter

Address PR comments

7f21db3

github-actions bot added the Stale label Oct 5, 2022

utpilla removed the Stale label Oct 10, 2022

github-actions bot added the Stale label Oct 18, 2022

reyang removed the Stale label Oct 19, 2022

utpilla added 3 commits October 20, 2022 16:06

Merge remote-tracking branch 'origin/main' into utpilla/Add-TLDTraceE…

58c474d

…xporter

Do not enable TLDTraceExporter; Add TODOs

f45f771

Remove unnecessary using statement

9157483

github-actions bot added the Stale label Oct 28, 2022

github-actions bot closed this Nov 5, 2022

utpilla added 2 commits January 6, 2023 23:54

Merge remote-tracking branch 'origin/main' into utpilla/Add-TLDTraceE…

a3fc35b

…xporter

Code changes

0d9f51c

utpilla reopened this Jan 7, 2023

More changes

0fdea54

utpilla removed the Stale label Jan 7, 2023

utpilla added 3 commits January 7, 2023 18:34

Add Benchmarks; JsonSerializer unit tests

4d7e856

Update JsonSerializer

3aa347e

Code refactoring

1eb682d

reyang approved these changes Jan 10, 2023

View reviewed changes

utpilla added 3 commits January 10, 2023 15:57

Fix CI

e29b378

Rename files

17edccb

Merge branch 'main' into utpilla/Add-TLDTraceExporter

a4168b7

utpilla merged commit cea38d3 into open-telemetry:main Jan 11, 2023

mmanciop pushed a commit to lumigo-io/opentelemetry-dotnet-contrib that referenced this pull request Jan 11, 2023

[Exporter.Geneva] Add TLDTraceExporter (open-telemetry#662)

cbe388e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Exporter.Geneva] Add TLDTraceExporter #662

[Exporter.Geneva] Add TLDTraceExporter #662

utpilla commented Sep 27, 2022 •

edited

Loading

codecov bot commented Sep 27, 2022 •

edited

Loading

utpilla Sep 27, 2022

utpilla Sep 27, 2022

reyang Sep 27, 2022 •

edited

Loading

utpilla Sep 27, 2022 •

edited

Loading

utpilla Oct 20, 2022

utpilla Sep 27, 2022

reyang Sep 27, 2022

utpilla Sep 27, 2022

utpilla Oct 20, 2022

github-actions bot commented Oct 5, 2022

github-actions bot commented Oct 18, 2022

github-actions bot commented Oct 28, 2022

github-actions bot commented Nov 5, 2022

[Exporter.Geneva] Add TLDTraceExporter #662

[Exporter.Geneva] Add TLDTraceExporter #662

Conversation

utpilla commented Sep 27, 2022 • edited Loading

Changes

TODOs:

Setup and usage:

codecov bot commented Sep 27, 2022 • edited Loading

Codecov Report

utpilla Sep 27, 2022

Choose a reason for hiding this comment

utpilla Sep 27, 2022

Choose a reason for hiding this comment

reyang Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

utpilla Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

utpilla Oct 20, 2022

Choose a reason for hiding this comment

utpilla Sep 27, 2022

Choose a reason for hiding this comment

reyang Sep 27, 2022

Choose a reason for hiding this comment

utpilla Sep 27, 2022

Choose a reason for hiding this comment

utpilla Oct 20, 2022

Choose a reason for hiding this comment

github-actions bot commented Oct 5, 2022

github-actions bot commented Oct 18, 2022

github-actions bot commented Oct 28, 2022

github-actions bot commented Nov 5, 2022

utpilla commented Sep 27, 2022 •

edited

Loading

codecov bot commented Sep 27, 2022 •

edited

Loading

reyang Sep 27, 2022 •

edited

Loading

utpilla Sep 27, 2022 •

edited

Loading