Skip to content
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

Step 1 - Use new Activity to Replace OT Span #660

Merged
merged 19 commits into from
May 13, 2020
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions samples/Exporters/Console/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public class Program
/// <param name="args">Arguments from command line.</param>
public static void Main(string[] args)
{
Parser.Default.ParseArguments<JaegerOptions, ZipkinOptions, PrometheusOptions, HttpClientOptions, ZPagesOptions, ConsoleOptions, OtlpOptions>(args)
Parser.Default.ParseArguments<JaegerOptions, ZipkinOptions, PrometheusOptions, HttpClientOptions, ZPagesOptions, ConsoleOptions, ConsoleActivityOptions, OtlpOptions>(args)
.MapResult(
(JaegerOptions options) => TestJaeger.Run(options.Host, options.Port),
(ZipkinOptions options) => TestZipkin.Run(options.Uri),
Expand All @@ -46,6 +46,7 @@ public static void Main(string[] args)
(RedisOptions options) => TestRedis.Run(options.Uri),
(ZPagesOptions options) => TestZPages.Run(),
(ConsoleOptions options) => TestConsole.Run(options),
(ConsoleActivityOptions options) => TestConsoleActivity.Run(options),
(OtlpOptions options) => TestOtlp.Run(options.Endpoint),
errs => 1);

Expand Down Expand Up @@ -105,10 +106,17 @@ internal class ZPagesOptions
[Verb("console", HelpText = "Specify the options required to test console exporter")]
internal class ConsoleOptions
{
[Option('p', "pretty", HelpText = "Specify if the output should be pretty printed (default: false)", Default = false)]
[Option('p', "pretty", HelpText = "Specify if the output should be pretty printed (default: true)", Default = true)]
public bool Pretty { get; set; }
}

[Verb("consoleactivity", HelpText = "Specify the options required to test console activity exporter")]
internal class ConsoleActivityOptions
{
[Option('p', "displayasjson", HelpText = "Specify if the output should be displayed as json or not (default: false)", Default = false)]
public bool DisplayAsJson { get; set; }
}

[Verb("otlp", HelpText = "Specify the options required to test OpenTelemetry Protocol (OTLP)")]
internal class OtlpOptions
{
Expand Down
23 changes: 19 additions & 4 deletions samples/Exporters/Console/TestConsole.cs
Original file line number Diff line number Diff line change
@@ -1,8 +1,22 @@
// <auto-generated/>
// <copyright file="TestConsole.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using OpenTelemetry.Trace.Configuration;
using OpenTelemetry.Exporter.Console;
using OpenTelemetry.Trace;
using OpenTelemetry.Trace.Configuration;

namespace Samples
{
Expand All @@ -13,14 +27,15 @@ internal static object Run(ConsoleOptions options)
// map test project settings to ConsoleExporterSetting
var exporterOptions = new ConsoleExporterOptions
{
Pretty = options.Pretty
Pretty = options.Pretty,
};

// create exporter
var exporter = new ConsoleExporter(exporterOptions);

// Create tracer
using var tracerFactory = TracerFactory.Create(builder => {
using var tracerFactory = TracerFactory.Create(builder =>
{
builder.AddProcessorPipeline(p => p.SetExporter(exporter));
});
var tracer = tracerFactory.GetTracer("console-test");
Expand Down
95 changes: 95 additions & 0 deletions samples/Exporters/Console/TestConsoleActivity.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
// <copyright file="TestConsoleActivity.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System;
using System.Diagnostics;
using OpenTelemetry.Exporter.Console;
using OpenTelemetry.Trace.Configuration;
using OpenTelemetry.Trace.Export;

namespace Samples
{
internal class TestConsoleActivity
{
internal static object Run(ConsoleActivityOptions options)
{
// Setup exporter
var exporterOptions = new ConsoleActivityExporterOptions
{
DisplayAsJson = options.DisplayAsJson,
};
var activityExporter = new ConsoleActivityExporter(exporterOptions);

// Setup processor
var activityProcessor = new SimpleActivityProcessor(activityExporter);

// Enable OpenTelemetry for the "source" named "MyCompany.MyProduct.MyWebServer".
OpenTelemetrySDK.EnableOpenTelemetry("MyCompany.MyProduct.MyWebServer", activityProcessor);

// Everything above this line is required only in Applications
// which decide to use OT.

// The following is generating activity.
// Libraries would simply write the following lines of code.
var source = new ActivitySource("MyCompany.MyProduct.MyWebServer");

// The below commented out line shows more likely code a in real world webserver.
// using (var parent = source.StartActivity("HttpIn", ActivityKind.Server, HttpContext.Request.Headers["traceparent"] ))
using (var parent = source.StartActivity("HttpIn", ActivityKind.Server))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really do not like this API - it's very easy to missing a null check and introduce a non-obvious error. It also goes against the OpenTelemetry spec that a no-op tracer / span is returned in such a way no code needs to change.

Copy link
Contributor

@tarekgh tarekgh May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's very easy to missing a null check and introduce a non-obvious error.

StartActivity is not different than any other API that can return a null value. we use the ? for simplifying the code. Usually, users adopt new APIs will look at the doc and usage patterns. Also, usually, anyone writes code like that, is going to test it. simple case testing will catch this problem very easily.

It also goes against the OpenTelemetry spec that a no-op tracer/span is returned in such a way no code needs to change.

There is no code that needs to change anyway. right? The code will be the same either there are listeners or not. please notice, Activity is a type that is used for many years. In our design, we wanted to serve both users already using Activity and OT scenarios.

CC @noahfalk

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @tarekgh - I didn't mean my comments to come across so negative, sorry. I'll try to be more constructive.

I guess my main issue is that it is significantly different in usage compared to the OpenTelemetry specification. The span & tracer interfaces are designed to be robust and easy to use, providing an efficient no-op design if configuration is incomplete.

The Activity API can return null and must be checked for manually every time a usr wishes to interact with it, which becomes error prone if using the shorthand null check. It also does not support non-string tags (see http status code) which deviates from the spec and will cause propagation issues between language implementations (all other languages allow non-strings).

Copy link
Contributor

@tarekgh tarekgh May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MikeGoldsmith I appreciate your feedback, it is helpful. So, don't hesitate to send more :-)

Yes, I understand there are some differences between the OT specs and the .NET APIs. We have been working hard to get them close and fill the gaps but at the same time, we are restricted by some other constraints. We had to come with a design that not confuses a lot of developers who currently using the Activity APIs and at the same allow the OT scenarios. Think about it as we are not strictly implementing the OT spec as it is but we are enabling all scenarios that OT can do.

We'll continue to look at the feedback and try to figure out how we can help more with that. Thanks again for your feedback.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't mean my comments to come across so negative

No worries at all @MikeGoldsmith, your initial feedback was straightforward and the objections were grounded. It was fine feedback : )

I guess my main issue is that it is significantly different in usage compared to the OpenTelemetry specification. The span & tracer interfaces are designed to be robust and easy to use, providing an efficient no-op design if configuration is incomplete

I agree, it is different. From my perspective deciding to reuse Activity APIs and integrate into the platform rather than creating a competing API has a lot of advantages, but one disadvantage is that it creates additional constraints on the design. In this case OT's premise is that it is easy and cheap to create a no-op Span so that nobody has to do a null check. We considered a no-op Activity, but didn't like the options:

  1. If the no-op Activity behaves like a normal Activity then it is stateful and we can't use a global singleton. This means we are paying for allocations and all the normal initialization. Some of the work unfortunately isn't that cheap.
  2. If the no-op Activity doesn't behave like a normal Activity we can make it cheap, but now we've created some risk that pre-existing code assets had assumptions about how Activity works which are invalidated. For example maybe the existing code assumes that properties can be read back after they are set, or that the timestamp of a started activity isn't 0.

So we ended at option 3, which was let the user see the null result. Our hope is that compiler nullable reference analysis makes it obvious when you've done the wrong thing, or if you are using an older compiler then code snippets, docs, and testing. Certainly its possible that this will be more problematic than we estimated so getting this kind of feedback is important (thanks!). So far we haven't heard much concern raised around this, but our overall feedback volume is probably still low.

It also does not support non-string tags

This one should probably be separated into its own issue. @cijothomas @reyang - have you guys experimented much with tags that would be non-string types in other languages? Certainly I see some potential for friction here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one should probably be separated into its own issue. @cijothomas @reyang - have you guys experimented much with tags that would be non-string types in other languages? Certainly I see some potential for friction here.

Yep. @cijothomas @tarekgh and I were chatting about this topic earlier today. We do see a good value of having non-string types.

{
parent?.AddTag("http.method", "GET");
parent?.AddTag("http.host", "MyHostName");
if (parent != null)
{
parent.DisplayName = "HttpIn DisplayName";
}

try
{
// Actual code to achieve the purpose of the library.
// For websebserver example, this would be calling
// user middlware pipeline.

// There can be child activities.
// In this example HttpOut is a child of HttpIn.
using (var child = source.StartActivity("HttpOut", ActivityKind.Client))
{
child?.AddTag("http.url", "www.mydependencyapi.com");
try
{
// do actual work.

child?.AddEvent(new ActivityEvent("sample activity event."));
child?.AddTag("http.status_code", "200");
}
catch (Exception)
{
child?.AddTag("http.status_code", "500");
}
}

parent?.AddTag("http.status_code", "200");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the spec it is an integer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup. Tags are currently string,string only. This is a drift from OT spec.

Copy link
Member

@reyang reyang May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think http.status_code should use parent?.SetCustomProperty("http.status_code", 200) rather than tag?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current adapters use Attributes for storing status_code with ("http.status_code", 200). Attributes support int, but its replacement Activity.Tags only support string.

On top of this, Spans have Status, which don't have a place in Activity yet - the suggestion was to use activity.SetCustomProperty, for the SpanStatus. This was not finalized as well, as there was plans to drop the Status from Span?.

Agree this needs to be finalized - do we need it for this PR or we can do it when more clarity is achieved?
My simplest Exporter is not looking at activity.CustomProperties. Would add it in subsequent PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tags are currently string,string only. This is a drift from OT spec.

It will be good if exporters (at least the ones on this repo) have consistent behavior in this regard, eg.: by default have every value as a string and optionally enable a best-effort to convert to some "supported" types by the exporter format.

... as there was plans to drop the Status from Span?.

is anybody actively pursuing that?

Copy link
Contributor

@tarekgh tarekgh May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the discussion here regarding supporting the OT Status type, then SetCustomProperty can be used here. But if we are talking about adding simple HTTP status code, then I believe using Tags will be more reasonable as it will log a simple code and no further information attached to it (e.g. description or even the thrown exception).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For HTTP the existing semantic conventions are already enough (and even the OTel Status can be derived from that). For others wanting to set span status or equivalent, we need further conventions to do the same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any pending work to support non-string values as activity tags?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any pending work to support non-string values as activity tags?

Yes. Tarek is working on proposals.

}
catch (Exception)
{
parent?.AddTag("http.status_code", "500");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add example with the result setting. I bet the most of implementation will need try{}catch to indicate that the activity failed

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If parent may be null (see parent?.), will the using statement fail?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Parent will be null if none is listening.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be a real pain, many many people will forgot the null check.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To sergey's qn - the using statement doesn't fail.
But any other place where parent is not nullchecked will fail. Hence the parent? usage throughout. Users are expected to do this, and this is unit testable easily.


return null;
}
}
}
97 changes: 97 additions & 0 deletions src/OpenTelemetry.Exporter.Console/ConsoleActivityExporter.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
// <copyright file="ConsoleActivityExporter.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Text.Json;
using System.Text.Json.Serialization;
using System.Threading;
using System.Threading.Tasks;
using OpenTelemetry.Trace.Export;

namespace OpenTelemetry.Exporter.Console
{
public class ConsoleActivityExporter : ActivityExporter
{
private readonly JsonSerializerOptions serializerOptions;
private bool displayAsJson;

public ConsoleActivityExporter(ConsoleActivityExporterOptions options)
{
this.serializerOptions = new JsonSerializerOptions()
{
WriteIndented = true,
};

this.displayAsJson = options.DisplayAsJson;

this.serializerOptions.Converters.Add(new JsonStringEnumConverter());
this.serializerOptions.Converters.Add(new ActivitySpanIdConverter());
this.serializerOptions.Converters.Add(new ActivityTraceIdConverter());
}

public override Task<ExportResult> ExportAsync(IEnumerable<Activity> activityBatch, CancellationToken cancellationToken)
{
foreach (var activity in activityBatch)
{
if (this.displayAsJson)
{
System.Console.WriteLine(JsonSerializer.Serialize(activity, this.serializerOptions));
}
else
{
System.Console.WriteLine("Activity ID - " + activity.Id);
if (!string.IsNullOrEmpty(activity.ParentId))
{
System.Console.WriteLine("Activity ParentId - " + activity.ParentId);
}

System.Console.WriteLine("Activity OperationName - " + activity.OperationName);
System.Console.WriteLine("Activity DisplayName - " + activity.DisplayName);
System.Console.WriteLine("Activity StartTime - " + activity.StartTimeUtc);
System.Console.WriteLine("Activity Duration - " + activity.Duration);
if (activity.Tags.Count() > 0)
{
System.Console.WriteLine("Activity Tags");
foreach (var tag in activity.Tags)
{
System.Console.WriteLine($"TagName: {tag.Key} TagValue: {tag.Value}");
}
}

if (activity.Events.Count() > 0)
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
{
System.Console.WriteLine("Activity Events");
foreach (var activityEvent in activity.Events)
{
System.Console.WriteLine($"Event Name: {activityEvent.Name} TimeStamp: {activityEvent.Timestamp}");
}
}

System.Console.WriteLine("\n");
}
}

return Task.FromResult(ExportResult.Success);
}

public override Task ShutdownAsync(CancellationToken cancellationToken)
{
return Task.CompletedTask;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// <copyright file="ConsoleActivityExporterOptions.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

namespace OpenTelemetry.Exporter.Console
{
public class ConsoleActivityExporterOptions
{
public bool DisplayAsJson { get; set; }
}
}
54 changes: 54 additions & 0 deletions src/OpenTelemetry/Trace/Configuration/OpenTelemetrySDK.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// <copyright file="OpenTelemetrySDK.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System.Diagnostics;
using OpenTelemetry.Trace.Export;

namespace OpenTelemetry.Trace.Configuration
{
public class OpenTelemetrySDK
bruno-garcia marked this conversation as resolved.
Show resolved Hide resolved
{
static OpenTelemetrySDK()
{
Activity.DefaultIdFormat = ActivityIdFormat.W3C;
Activity.ForceDefaultIdFormat = true;
}

/// <summary>
/// Enables OpenTelemetry for the ActivitySource with given name.
/// </summary>
/// <param name="source">Name of the ActivitySource.</param>
/// <param name="activityProcessor">ActivityProcessor which receives activity start/stop notifications.</param>
/// <remarks>
/// A trivial implementation. Most logic from TracerBuilder will be ported here.
/// </remarks>
public static void EnableOpenTelemetry(string source, ActivityProcessor activityProcessor)
{
ActivityListener listener = new ActivityListener
{
ActivityStarted = activityProcessor.OnStart,
ActivityStopped = activityProcessor.OnEnd,
ShouldListenTo = (activitySource) => activitySource.Name.Equals(source),

// The following parameters are not used now.
GetRequestedDataUsingParentId = (ref ActivityCreationOptions<string> options) => ActivityDataRequest.AllData,
GetRequestedDataUsingContext = (ref ActivityCreationOptions<ActivityContext> options) => ActivityDataRequest.AllData,
};

ActivitySource.AddActivityListener(listener);
}
}
}
Loading