-
Notifications
You must be signed in to change notification settings - Fork 150
[Tracer] Add new process tags #7651
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
Changes from all commits
231f0bd
49c47fb
f252a35
415a4f4
dab0976
1899d1e
0226c49
99eba25
cc4b720
b49fc26
39083e8
bcb45ad
c941b82
e4b5c58
206784b
2ff6f91
b02cb33
cab50d8
fb6ffdb
8742240
2d21dd6
d0902d9
208c05e
b6b0fde
6238eba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| // <copyright file="ProcessTags.cs" company="Datadog"> | ||
| // Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License. | ||
| // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc. | ||
| // </copyright> | ||
|
|
||
| #nullable enable | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.IO; | ||
| using System.Text; | ||
| using Datadog.Trace.Configuration; | ||
| using Datadog.Trace.Processors; | ||
| using Datadog.Trace.Util; | ||
|
|
||
| namespace Datadog.Trace; | ||
|
|
||
| internal static class ProcessTags | ||
| { | ||
| public const string EntrypointName = "entrypoint.name"; | ||
| public const string EntrypointBasedir = "entrypoint.basedir"; | ||
| public const string EntrypointWorkdir = "entrypoint.workdir"; | ||
|
|
||
| private static readonly Lazy<string> LazySerializedTags = new(GetSerializedTags); | ||
|
|
||
| public static string SerializedTags | ||
| { | ||
| get => LazySerializedTags.Value; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// From the full path of a directory, get the name of the leaf directory. | ||
| /// </summary> | ||
| private static string GetLastPathSegment(string directoryPath) | ||
| { | ||
| // Path.GetFileName returns an empty string if the path ends with a '/'. | ||
| // We could use Path.TrimEndingDirectorySeparator instead of the trim here, but it's not available on .NET Framework | ||
| return Path.GetFileName(directoryPath.TrimEnd('\\').TrimEnd('/')); | ||
|
Comment on lines
+37
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is going to allocate multiple times, we can use ReadOnlySpan on netcore3.1+ to reduce the allocations #if NETCOREAPP3_1_OR_GREATER
return Path.GetFileName(directoryPath.AsSpan().TrimEnd('\\').TrimEnd('/'));
#else
return Path.GetFileName(directoryPath.TrimEnd('\\').TrimEnd('/'));
#endifThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd say the same thing as as said to Lucas above, sick trick, but do we really want to start having different code path for different fwk in there, given that this code runs only once ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's fair. I would but meh 😄 |
||
| } | ||
|
|
||
| private static string GetSerializedTags() | ||
| { | ||
| // ⚠️ make sure entries are added in alphabetical order of keys | ||
| var tags = new List<KeyValuePair<string, string?>> | ||
| { | ||
| new(EntrypointBasedir, GetLastPathSegment(AppContext.BaseDirectory)), | ||
| new(EntrypointName, EntryAssemblyLocator.GetEntryAssembly()?.EntryPoint?.DeclaringType?.FullName), | ||
| // workdir can be changed by the code, but we consider that capturing the value when this is called is good enough | ||
| new(EntrypointWorkdir, GetLastPathSegment(Environment.CurrentDirectory)) | ||
| }; | ||
vandonr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // then normalize values and put all tags in a string | ||
| var serializedTags = StringBuilderCache.Acquire(); | ||
| foreach (var kvp in tags) | ||
| { | ||
| if (!string.IsNullOrEmpty(kvp.Value)) | ||
| { | ||
| serializedTags.Append($"{kvp.Key}:{NormalizeTagValue(kvp.Value!)},"); | ||
| } | ||
| } | ||
|
|
||
| serializedTags.Remove(serializedTags.Length - 1, length: 1); // remove last comma | ||
| return StringBuilderCache.GetStringAndRelease(serializedTags); | ||
| } | ||
|
|
||
| private static string NormalizeTagValue(string tagValue) | ||
| { | ||
| // TraceUtil.NormalizeTag does almost exactly what we want, except it allows ':', | ||
| // which we don't want because we use it as a key/value separator. | ||
| return TraceUtil.NormalizeTag(tagValue).Replace(oldChar: ':', newChar: '_'); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| // <copyright file="ProcessTagsTests.cs" company="Datadog"> | ||
| // Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License. | ||
| // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc. | ||
| // </copyright> | ||
|
|
||
| using System.Collections.Specialized; | ||
| using System.Linq; | ||
| using System.Threading.Tasks; | ||
| using Datadog.Trace.Agent; | ||
| using Datadog.Trace.Configuration; | ||
| using Datadog.Trace.TestHelpers; | ||
| using Datadog.Trace.TestHelpers.TestTracer; | ||
| using FluentAssertions; | ||
| using Xunit; | ||
|
|
||
| namespace Datadog.Trace.IntegrationTests.Tagging; | ||
|
|
||
| public class ProcessTagsTests | ||
| { | ||
| private readonly MockApi _testApi; | ||
|
|
||
| public ProcessTagsTests() | ||
| { | ||
| _testApi = new MockApi(); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(true)] | ||
| [InlineData(false)] | ||
| public async Task ProcessTags_Only_In_First_Span(bool enabled) | ||
| { | ||
| var settings = new TracerSettings(new NameValueConfigurationSource(new NameValueCollection { { ConfigurationKeys.PropagateProcessTags, enabled ? "true" : "false" } })); | ||
| var agentWriter = new AgentWriter(_testApi, statsAggregator: null, statsd: null, automaticFlush: false); | ||
| await using var tracer = TracerHelper.Create(settings, agentWriter); | ||
|
|
||
| using (tracer.StartActiveInternal("A")) | ||
| using (tracer.StartActiveInternal("AA")) | ||
| { | ||
| } | ||
|
|
||
| // other trace | ||
| using (tracer.StartActiveInternal("B")) | ||
| using (tracer.StartActiveInternal("BB")) | ||
| { | ||
| } | ||
|
|
||
| await tracer.FlushAsync(); | ||
| var traceChunks = _testApi.Wait(); | ||
|
|
||
| traceChunks.Should().HaveCount(2); // 2 (small) traces = 2 chunks | ||
| if (enabled) | ||
| { | ||
| // process tags written only to first span of first chunk | ||
| traceChunks[0][0].Tags.Should().ContainKey(Tags.ProcessTags); | ||
| } | ||
| else | ||
| { | ||
| traceChunks[0][0].Tags.Should().NotContainKey(Tags.ProcessTags); | ||
| } | ||
|
|
||
| traceChunks.SelectMany(x => x) // flatten | ||
| .Skip(1) // exclude first item that we just checked above | ||
| .Should() | ||
| .AllSatisfy(s => s.Tags.Should().NotContainKey(Tags.ProcessTags)); | ||
| } | ||
| } |
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.
Are process tags supposed to be added to CI Visibility payloads? Because they have their own formatters if so
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.
hmm 🤔 that's a good question, does CI visibility have a notion of service name too ? (if yes yes, if no no)
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.
confirmed that we don't need them on CI visibility
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.
Ci Vis does have a notion of service name, yes, but I guess this isn't an issue 🤷♂️