Added Console Trace link writer #222

martinjt · 2022-07-23T11:06:23Z

Adds a link exporter that writes all root spans to the console with a link to honeycomb for viewing it.

MikeGoldsmith

Thanks for the contribution 👍🏻

I like what you've done and I think it's heading in the right direction. I've left some comments to make use of existing Options values and suggested some other things.

I've focused on external config & behaviour as internals can be updated later and this is likely a debug tool rather than being activated in production systems.

MikeGoldsmith · 2022-07-25T10:03:21Z

src/Honeycomb.OpenTelemetry/ConsoleLinksExporter.cs

+namespace Honeycomb.OpenTelemetry
+{
+    /// <summary>
+    /// Writes links to the Honeycomb for all root spans


Suggested change

/// Writes links to the Honeycomb for all root spans

/// Writes links to the Console for all root spans

MikeGoldsmith · 2022-07-25T10:10:27Z

src/Honeycomb.OpenTelemetry/ConsoleLinksExporter.cs

+    /// </summary>
+    public class ConsoleLinkExporter : BaseExporter<Activity>
+    {
+        private readonly HoneycombOptions _options;


We only need the service name outside of init, let's capture that instead directly instead of keeping a reference to the whole options object.

MikeGoldsmith · 2022-07-25T10:11:30Z

src/Honeycomb.OpenTelemetry/ConsoleLinksExporter.cs

+    public class ConsoleLinkExporter : BaseExporter<Activity>
+    {
+        private readonly HoneycombOptions _options;
+        private string _teamSlug;


I think getting the team and environment slugs could be reusable so may end up being passed in instead of being resolved inside this span processor.

I don't think that we should be calling out via http as part of startup in any other environment than production. It's an async call in a sync method, and I don't like it all. As it stands, because it's in the ConsoleLinkExporter, it won't be triggered unless you ask for tracelinks.

I think that's probably an internal decision too, so we can refactor that as and when we need it.

MikeGoldsmith · 2022-07-25T10:15:24Z

src/Honeycomb.OpenTelemetry/ConsoleLinksExporter.cs

+            var httpClient = new HttpClient();
+            httpClient.BaseAddress = new Uri("https://api.honeycomb.io/1/auth");
+            httpClient.DefaultRequestHeaders.Add("X-Honeycomb-Team", _options.ApiKey);
+
+            var response = httpClient.GetAsync("").GetAwaiter().GetResult();
+            if (!response.IsSuccessStatusCode) {
+                Console.WriteLine("Didn't get a valid response from Honeycomb");
+                return;
+            }


We can use _options.TracesEndpoint instead of hardcoding.

Suggested change

var httpClient = new HttpClient();

httpClient.BaseAddress = new Uri("https://api.honeycomb.io/1/auth");

httpClient.DefaultRequestHeaders.Add("X-Honeycomb-Team", _options.ApiKey);

var response = httpClient.GetAsync("").GetAwaiter().GetResult();

if (!response.IsSuccessStatusCode) {

Console.WriteLine("Didn't get a valid response from Honeycomb");

return;

}

var httpClient = new HttpClient();

httpClient.BaseAddress = new Uri(_options.TracesEndpoint);

httpClient.DefaultRequestHeaders.Add("X-Honeycomb-Team", _options.ApiKey);

var response = httpClient.GetAsync("1/auth").GetAwaiter().GetResult();

if (!response.IsSuccessStatusCode) {

Console.WriteLine("Didn't get a valid response from Honeycomb");

return;

}

Given that someone can override the traces endpoint, is that an issue? My worry would be people overriding that, and not having the auth API... maybe I'm being a little over cautious?

MikeGoldsmith · 2022-07-25T10:16:15Z

src/Honeycomb.OpenTelemetry/ConsoleLinksExporter.cs

+            var authResponse = JsonSerializer.Deserialize<AuthResponse>(responseString);
+            _environmentSlug = authResponse.Environment.Slug;
+            _teamSlug = authResponse.Team.Slug;
+            if (string.IsNullOrEmpty(_environmentSlug) ||


We can generate trace links for classic datasets, it just needs to omit the environment segments.

How would we detect if it's a classic dataset?

How would we detect if it's a classic dataset?

Classic (nee Legacy) API key is 32 characters:

honeycomb-opentelemetry-dotnet/src/Honeycomb.OpenTelemetry/HoneycombOptions.cs

Line 56 in 5317498

internal bool IsTracesLegacyKey()

MikeGoldsmith · 2022-07-25T10:20:12Z

src/Honeycomb.OpenTelemetry/ConsoleLinksExporter.cs

+            {
+                if (string.IsNullOrEmpty(activity.ParentId))
+                {
+                    Console.WriteLine($"Trace Emitted for {activity.DisplayName}");


Would be nice to expand this in the future to write to an ILogger instead of directly to console. Then users can select where they want these to appear.

I actually don't want it going through the logging framework, as that could take it into other pipelines, and this is specifically for local development.

MikeGoldsmith · 2022-07-25T10:21:03Z

src/Honeycomb.OpenTelemetry/EnvironmentOptions.cs

@@ -16,6 +16,7 @@ internal class EnvironmentOptions
        private const string SampleRateKey = "HONEYCOMB_SAMPLE_RATE";
        private const string ServiceNameKey = "SERVICE_NAME";
        private const string ServiceVersionKey = "SERVICE_VERSION";
+        private const string WriteTraceLinksToConsoleKey = "WRITE_TRACE_LINKS_TO_CONSOLE";


This feels long, maybe LOG_TRACE_LINKS. If we do go along the route of using an ILogger in the future, these trace links may go elsewhere.

using Console is more universal I think, and also keeps it specific to enviroments where you have access to the console.

in zoom convo, we thought ENABLE_LOCAL_VISUALIZATIONS would work nicely, as a more generic "mode" setting that is not too specific with what we're writing where

MikeGoldsmith · 2022-07-25T10:21:21Z

src/Honeycomb.OpenTelemetry/HoneycombOptions.cs

+        /// <summary>
+        /// Write links to honeycomb traces as they come in
+        /// </summary>
+        public bool WriteTraceLinksToConsole


Should match env var above.

MikeGoldsmith · 2022-07-25T10:21:43Z

src/Honeycomb.OpenTelemetry/HoneycombOptions.cs

@@ -193,7 +203,8 @@ public string MetricsEndpoint
        /// (Optional) Options delegate to configure StackExchange.Redis instrumentation.
        /// </summary>
        public Action<StackExchangeRedisCallsInstrumentationOptions>
-            ConfigureStackExchangeRedisClientInstrumentationOptions { get; set; }
+            ConfigureStackExchangeRedisClientInstrumentationOptions
+        { get; set; }


Was this intentional, or did your IDE move this?

MikeGoldsmith · 2022-07-25T10:21:54Z

src/Honeycomb.OpenTelemetry/HoneycombOptions.cs

@@ -220,6 +231,7 @@ public string MetricsEndpoint
            { "--honeycomb-traces-endpoint", "tracesendpoint" },
            { "--honeycomb-metrics-endpoint", "metricsendpoint" },
            { "--honeycomb-samplerate", "samplerate" },
+            { "--honeycomb-write-trace-links-to-console", "writetracelinkstoconsole" },


Should match env var

martinjt

Fixed feedback

martinjt · 2022-07-25T17:41:38Z

src/Honeycomb.OpenTelemetry/EnvironmentOptions.cs

@@ -16,6 +16,7 @@ internal class EnvironmentOptions
        private const string SampleRateKey = "HONEYCOMB_SAMPLE_RATE";
        private const string ServiceNameKey = "SERVICE_NAME";
        private const string ServiceVersionKey = "SERVICE_VERSION";
+        private const string WriteTraceLinksToConsoleKey = "WRITE_TRACE_LINKS_TO_CONSOLE";


using Console is more universal I think, and also keeps it specific to enviroments where you have access to the console.

martinjt · 2022-07-25T17:45:40Z

src/Honeycomb.OpenTelemetry/ConsoleLinksExporter.cs

+    public class ConsoleLinkExporter : BaseExporter<Activity>
+    {
+        private readonly HoneycombOptions _options;
+        private string _teamSlug;


I don't think that we should be calling out via http as part of startup in any other environment than production. It's an async call in a sync method, and I don't like it all. As it stands, because it's in the ConsoleLinkExporter, it won't be triggered unless you ask for tracelinks.

I think that's probably an internal decision too, so we can refactor that as and when we need it.

martinjt · 2022-07-25T18:10:42Z

src/Honeycomb.OpenTelemetry/ConsoleLinksExporter.cs

+            var httpClient = new HttpClient();
+            httpClient.BaseAddress = new Uri("https://api.honeycomb.io/1/auth");
+            httpClient.DefaultRequestHeaders.Add("X-Honeycomb-Team", _options.ApiKey);
+
+            var response = httpClient.GetAsync("").GetAwaiter().GetResult();
+            if (!response.IsSuccessStatusCode) {
+                Console.WriteLine("Didn't get a valid response from Honeycomb");
+                return;
+            }


Given that someone can override the traces endpoint, is that an issue? My worry would be people overriding that, and not having the auth API... maybe I'm being a little over cautious?

martinjt · 2022-07-25T18:11:10Z

src/Honeycomb.OpenTelemetry/ConsoleLinksExporter.cs

+            var authResponse = JsonSerializer.Deserialize<AuthResponse>(responseString);
+            _environmentSlug = authResponse.Environment.Slug;
+            _teamSlug = authResponse.Team.Slug;
+            if (string.IsNullOrEmpty(_environmentSlug) ||


How would we detect if it's a classic dataset?

martinjt · 2022-07-25T18:11:45Z

src/Honeycomb.OpenTelemetry/ConsoleLinksExporter.cs

+            {
+                if (string.IsNullOrEmpty(activity.ParentId))
+                {
+                    Console.WriteLine($"Trace Emitted for {activity.DisplayName}");


I actually don't want it going through the logging framework, as that could take it into other pipelines, and this is specifically for local development.

martinjt · 2022-07-25T18:13:22Z

src/Honeycomb.OpenTelemetry/ConsoleLinksExporter.cs

+                    Console.WriteLine($"Trace Emitted for {activity.DisplayName}");
+                    Console.WriteLine($"TraceLink: {GetTraceLink(activity.TraceId.ToString())}");


Keep in mind that this is just for the root span, not all spans, so even in large volume applications, we should only see a small amount of spans.

1 line is actually pretty long as the url is already quite long, which is why I extended it to 2. Happy to hear thoughts though, I'm not 100% convinced this is right.

martinjt · 2022-07-25T18:14:15Z

src/Honeycomb.OpenTelemetry/ConsoleLinksExporter.cs

+        private string GetTraceLink(string traceId)
+        {
+            var dataset = _options.ServiceName;   
+            return $"http://ui.honeycomb.io/{_teamSlug}/environments/{_environmentSlug}/datasets/{dataset}/trace?trace_id={traceId}";


If I have a way of knowing that it's a classic just from the ApiKey or settings, then I'm happy to add it.

MikeGoldsmith

This looks good - thanks @martinjt 👍🏻

I'm happy with the changes to external API (env var / settings.json) and what it outputs. The remainder is internal logic and can be iterated on as we wish.

vreynolds · 2022-07-28T17:43:20Z

Circle is being weird, but I triggered the run by creating another branch off this PR.

There's an error trying to run tests:
C:\Program Files\dotnet\sdk\5.0.406\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.TargetFrameworkInference.targets(141,5): error NETSDK1045: The current .NET SDK does not support targeting .NET 6.0. Either target .NET 5.0 or lower, or use a version of the .NET SDK that supports .NET 6.0. [C:\Users\circleci\project\test\Honeycomb.OpenTelemetry.Tests\Honeycomb.OpenTelemetry.Tests.csproj]

vreynolds · 2022-07-28T17:54:36Z

Looks like Circle may not have .NET 6 in their orb

cartermp · 2022-07-28T18:10:22Z

STILL?! ugh

martinjt · 2022-07-28T21:28:54Z

That's my bad... I didn't mean to upgrade the projects to .NET 6. I'll revert

…

On Thu, 28 Jul 2022, 19:10 Phillip Carter, ***@***.***> wrote: STILL?! ugh — Reply to this email directly, view it on GitHub <#222 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAM66AYSEZSRS563N2D3UIDVWLEJRANCNFSM54N2ZTTQ> . You are receiving this because you were mentioned.Message ID: ***@***.*** com>

cartermp · 2022-07-28T21:54:23Z

It's quite reasonable to do that, though. Blegh. It also means having a minimal apis example can't be done yet either. :shakes-fist-at-circle-ci:

martinjt · 2022-07-29T10:40:17Z

I've added an install step to the CI build @cartermp , happy to revert to .NET 5 if we need to though. I can't trigger the build, so let me know if it works.

martinjt · 2022-07-29T12:22:28Z

Reverted this to .NET 5.0 as it was only the test project at this stage.

I'll investigate separately how we add .NET 6.0 inside the pipeline.

martinjt added 2 commits July 23, 2022 12:04

Added Console Trace link writer

65addf5

Get Team/Environment from Auth API and added settings

1188dc0

MikeGoldsmith requested changes Jul 25, 2022

View reviewed changes

MikeGoldsmith assigned martinjt Jul 25, 2022

MikeGoldsmith added type: enhancement New feature or request version: bump minor A PR that adds behavior, but is backwards-compatible. labels Jul 25, 2022

martinjt commented Jul 25, 2022

View reviewed changes

martinjt added 3 commits July 25, 2022 19:18

feedback fixed

aeaf146

more feedback

8ebcf77

updated based on feedback

b99df48

martinjt marked this pull request as ready for review July 26, 2022 11:05

martinjt requested a review from a team July 26, 2022 11:05

MikeGoldsmith approved these changes Jul 26, 2022

View reviewed changes

Install .NET 6.0 as part of build

a12d642

martinjt added 9 commits July 29, 2022 11:54

Add .NET 6 to path

add5dbd

update build scripts

fc9b5e0

Removed path setting

845509d

Add path update to use .NET 6.0

506fff4

correct path syntax

54cbea1

test whether path is being reset

b4c727e

fixed inline syntax

9f6bc81

2nd attempt to fix syntax!!

05a5768

removed net6

4c36706

MikeGoldsmith approved these changes Aug 1, 2022

View reviewed changes

MikeGoldsmith merged commit 5730151 into honeycombio:main Aug 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Console Trace link writer #222

Added Console Trace link writer #222

martinjt commented Jul 23, 2022

MikeGoldsmith left a comment

MikeGoldsmith Jul 25, 2022

MikeGoldsmith Jul 25, 2022

MikeGoldsmith Jul 25, 2022

martinjt Jul 25, 2022

MikeGoldsmith Jul 25, 2022

martinjt Jul 25, 2022

MikeGoldsmith Jul 25, 2022

martinjt Jul 25, 2022

JamieDanielson Jul 25, 2022

MikeGoldsmith Jul 25, 2022

martinjt Jul 25, 2022

MikeGoldsmith Jul 25, 2022

martinjt Jul 25, 2022

vreynolds Jul 25, 2022 •

edited

Loading

MikeGoldsmith Jul 25, 2022

MikeGoldsmith Jul 25, 2022

MikeGoldsmith Jul 25, 2022

martinjt left a comment

martinjt Jul 25, 2022

martinjt Jul 25, 2022

martinjt Jul 25, 2022

martinjt Jul 25, 2022

martinjt Jul 25, 2022

martinjt Jul 25, 2022

martinjt Jul 25, 2022

MikeGoldsmith left a comment

vreynolds commented Jul 28, 2022

vreynolds commented Jul 28, 2022

cartermp commented Jul 28, 2022

martinjt commented Jul 28, 2022 via email

cartermp commented Jul 28, 2022

martinjt commented Jul 29, 2022

martinjt commented Jul 29, 2022

	/// Writes links to the Honeycomb for all root spans
	/// Writes links to the Console for all root spans

		Console.WriteLine($"Trace Emitted for {activity.DisplayName}");
		Console.WriteLine($"TraceLink: {GetTraceLink(activity.TraceId.ToString())}");

Added Console Trace link writer #222

Added Console Trace link writer #222

Conversation

martinjt commented Jul 23, 2022

MikeGoldsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vreynolds Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinjt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeGoldsmith left a comment

Choose a reason for hiding this comment

vreynolds commented Jul 28, 2022

vreynolds commented Jul 28, 2022

cartermp commented Jul 28, 2022

martinjt commented Jul 28, 2022 via email

cartermp commented Jul 28, 2022

martinjt commented Jul 29, 2022

martinjt commented Jul 29, 2022

vreynolds Jul 25, 2022 •

edited

Loading