Skip to content

Commit

Permalink
wip: used to review approach
Browse files Browse the repository at this point in the history
  • Loading branch information
longility committed Apr 25, 2020
1 parent 93fa3f9 commit f700828
Show file tree
Hide file tree
Showing 11 changed files with 157 additions and 145 deletions.
18 changes: 9 additions & 9 deletions src/OpenTelemetry.Adapter.Dependencies/DependenciesAdapter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,23 +29,23 @@ public class DependenciesAdapter : IDisposable
/// <summary>
/// Initializes a new instance of the <see cref="DependenciesAdapter"/> class.
/// </summary>
/// <param name="tracerFactory">Tracer factory to get a tracer from.</param>
/// <param name="tracerProvider">Tracer provider to get a tracer from.</param>
/// <param name="httpOptions">Http configuration options.</param>
/// <param name="sqlOptions">Sql configuration options.</param>
public DependenciesAdapter(TracerFactoryBase tracerFactory, HttpClientAdapterOptions httpOptions = null, SqlClientAdapterOptions sqlOptions = null)
public DependenciesAdapter(ITracerProvider tracerProvider, HttpClientAdapterOptions httpOptions = null, SqlClientAdapterOptions sqlOptions = null)
{
if (tracerFactory == null)
if (tracerProvider == null)
{
throw new ArgumentNullException(nameof(tracerFactory));
throw new ArgumentNullException(nameof(tracerProvider));
}

var assemblyVersion = typeof(DependenciesAdapter).Assembly.GetName().Version;

var httpClientListener = new HttpClientAdapter(tracerFactory.GetTracer(nameof(HttpClientAdapter), "semver:" + assemblyVersion), httpOptions ?? new HttpClientAdapterOptions());
var httpWebRequestAdapter = new HttpWebRequestAdapter(tracerFactory.GetTracer(nameof(HttpWebRequestAdapter), "semver:" + assemblyVersion), httpOptions ?? new HttpClientAdapterOptions());
var azureClientsListener = new AzureClientsAdapter(tracerFactory.GetTracer(nameof(AzureClientsAdapter), "semver:" + assemblyVersion));
var azurePipelineListener = new AzurePipelineAdapter(tracerFactory.GetTracer(nameof(AzurePipelineAdapter), "semver:" + assemblyVersion));
var sqlClientListener = new SqlClientAdapter(tracerFactory.GetTracer(nameof(AzurePipelineAdapter), "semver:" + assemblyVersion), sqlOptions ?? new SqlClientAdapterOptions());
var httpClientListener = new HttpClientAdapter(tracerProvider.GetTracer(nameof(HttpClientAdapter), "semver:" + assemblyVersion), httpOptions ?? new HttpClientAdapterOptions());
var httpWebRequestAdapter = new HttpWebRequestAdapter(tracerProvider.GetTracer(nameof(HttpWebRequestAdapter), "semver:" + assemblyVersion), httpOptions ?? new HttpClientAdapterOptions());
var azureClientsListener = new AzureClientsAdapter(tracerProvider.GetTracer(nameof(AzureClientsAdapter), "semver:" + assemblyVersion));
var azurePipelineListener = new AzurePipelineAdapter(tracerProvider.GetTracer(nameof(AzurePipelineAdapter), "semver:" + assemblyVersion));
var sqlClientListener = new SqlClientAdapter(tracerProvider.GetTracer(nameof(AzurePipelineAdapter), "semver:" + assemblyVersion), sqlOptions ?? new SqlClientAdapterOptions());

this.adapters.Add(httpClientListener);
this.adapters.Add(httpWebRequestAdapter);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public StackExchangeRedisCallsAdapter(Tracer tracer)

this.cancellationTokenSource = new CancellationTokenSource();
this.cancellationToken = this.cancellationTokenSource.Token;
var spanType = typeof(TracerFactory).Assembly.GetType("OpenTelemetry.Trace.SpanSdk");
var spanType = typeof(TracerProviderSdk).Assembly.GetType("OpenTelemetry.Trace.SpanSdk");

this.spanEndTimestampInfo = spanType?.GetProperty("EndTimestamp");
if (this.spanEndTimestampInfo == null)
Expand Down
32 changes: 32 additions & 0 deletions src/OpenTelemetry.Api/Trace/ITracerProvider.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// <copyright file="ITracerProvider.cs" company="OpenTelemetry Authors">
// Copyright 2018, 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.Trace
{
/// <summary>
/// Creates Tracers for an instrumentation library.
/// </summary>
public interface ITracerProvider

This comment has been minimized.

Copy link
@cijothomas

cijothomas Apr 26, 2020

in general, we try to use abstract classes instead of interfaces, so that if we ever need to extend this, we can do that without breaking existing users.

This comment has been minimized.

Copy link
@longility

longility Apr 26, 2020

Author Owner

Can you elaborate your comment with possible examples? I'm having a hard time seeing the difference related to breaking existing users.

Are you maybe more talking about if we extend, we would have default behaviors that don't immediately require users to implement?

This comment has been minimized.

Copy link
@cijothomas

cijothomas Apr 26, 2020

Yes. If we introduce a new method to an interface, existing customers are immediately broken. If its abstract class, we can add a new method, and provide default impl, so existing users are not broken.

{
/// <summary>
/// Returns an Tracer for a given name and version.
/// </summary>
/// <param name="name">Name of the instrumentation library.</param>
/// <param name="version">Version of the instrumentation library (optional).</param>
/// <returns>Tracer for the given name and version information.</returns>
Tracer GetTracer(string name, string version = null);
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// <copyright file="BlankSpan.cs" company="OpenTelemetry Authors">
// <copyright file="NoopSpan.cs" company="OpenTelemetry Authors">
// Copyright 2018, OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
Expand All @@ -18,19 +18,10 @@
namespace OpenTelemetry.Trace
{
/// <summary>
/// Blank span.
/// No-op span.
/// </summary>
internal sealed class BlankSpan : TelemetrySpan
public sealed class NoopSpan : TelemetrySpan
{
/// <summary>
/// Blank span instance.
/// </summary>
public static readonly BlankSpan Instance = new BlankSpan();

private BlankSpan()
{
}

/// <inheritdoc />
public override SpanContext Context => default;

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// <copyright file="ProxyTracer.cs" company="OpenTelemetry Authors">
// <copyright file="NoopTracer.cs" company="OpenTelemetry Authors">
// Copyright 2018, OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
Expand All @@ -17,58 +17,49 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Threading;
using OpenTelemetry.Context.Propagation;

namespace OpenTelemetry.Trace
{
/// <summary>
/// No-op tracer.
/// </summary>
internal sealed class ProxyTracer : Tracer
public sealed class NoopTracer : Tracer

This comment has been minimized.

Copy link
@cijothomas

cijothomas Apr 26, 2020

the class was aptly named Proxy originally as it proxies to the real traces if available, else noop. It had method to update the realtracer, so that libraries can get updated realtraces when one becomes available.

How does the new one handle this scenario? Looks like if a library got NoOpTracer before SDK completed initialization, then library will have to live with NoOpTracer forever. This would be incorrect.

This comment has been minimized.

Copy link
@longility

longility Apr 26, 2020

Author Owner

You mentioned a behavior I was not expecting so I would want to clarify and make sure.

I was expecting that initialization would be completed, before get tracer gets called. However, I don't think that is too big of a problem if the tracer is more transient (short lived). Is that not true? Is there a scenario where a process is hanging onto a tracer for a long time that we need to support switch over if Tracer implementation changes?

This comment has been minimized.

Copy link
@cijothomas

cijothomas Apr 26, 2020

We can't be sure that SDK initialization is complete before libraries try to get tracerproviders/tracer. So libraries start with NoOps, and once initialization is complete, libraries get the correct trace. (This was handled by the current proxy tracer)

{
private static readonly IDisposable NoopScope = new NoopDisposable();

private Tracer realTracer;
private readonly TelemetrySpan noopSpan = new NoopSpan();

/// <inheritdoc/>
public override TelemetrySpan CurrentSpan => this.realTracer?.CurrentSpan ?? BlankSpan.Instance;
public override TelemetrySpan CurrentSpan => this.noopSpan;

/// <inheritdoc/>
public override IDisposable WithSpan(TelemetrySpan span, bool endOnDispose)
{
return this.realTracer != null ? this.realTracer.WithSpan(span, endOnDispose) : NoopScope;
return NoopScope;
}

/// <inheritdoc/>
public override TelemetrySpan StartRootSpan(string operationName, SpanKind kind, SpanCreationOptions options)
{
return this.realTracer != null ? this.realTracer.StartRootSpan(operationName, kind, options) : BlankSpan.Instance;
return this.noopSpan;
}

/// <inheritdoc/>
public override TelemetrySpan StartSpan(string operationName, TelemetrySpan parent, SpanKind kind, SpanCreationOptions options)
{
return this.realTracer != null ? this.realTracer.StartSpan(operationName, parent, kind, options) : BlankSpan.Instance;
return this.noopSpan;
}

/// <inheritdoc/>
public override TelemetrySpan StartSpan(string operationName, in SpanContext parent, SpanKind kind, SpanCreationOptions options)
{
return this.realTracer != null ? this.realTracer.StartSpan(operationName, parent, kind, options) : BlankSpan.Instance;
return this.noopSpan;
}

/// <inheritdoc/>
public override TelemetrySpan StartSpanFromActivity(string operationName, Activity activity, SpanKind kind, IEnumerable<Link> links)
{
return this.realTracer != null ? this.realTracer.StartSpanFromActivity(operationName, activity, kind, links) : BlankSpan.Instance;
}

public void UpdateTracer(Tracer realTracer)
{
if (this.realTracer != null)
{
return;
}

// just in case user calls init concurrently
Interlocked.CompareExchange(ref this.realTracer, realTracer, null);
return this.noopSpan;
}

private class NoopDisposable : IDisposable
Expand Down
33 changes: 33 additions & 0 deletions src/OpenTelemetry.Api/Trace/NoopTracerProvider.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// <copyright file="NoopTracerProvider.cs" company="OpenTelemetry Authors">
// Copyright 2018, 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;

namespace OpenTelemetry.Trace
{
/// <summary>
/// No-op tracer provider.
/// </summary>
public class NoopTracerProvider : ITracerProvider
{
private readonly NoopTracer tracer = new NoopTracer();

/// <inheritdoc/>
public Tracer GetTracer(string name, string version = null)
{
return this.tracer;
}
}
}
84 changes: 0 additions & 84 deletions src/OpenTelemetry.Api/Trace/TracerFactoryBase.cs

This file was deleted.

46 changes: 46 additions & 0 deletions src/OpenTelemetry.Api/Trace/TracerProvider.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// <copyright file="TracerProvider.cs" company="OpenTelemetry Authors">
// Copyright 2018, 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.Trace
{
/// <summary>
/// Creates Tracers for an instrumentation library.
/// </summary>
public class TracerProvider
{
/// <summary>
/// Gets or sets the global instance of <see cref="ITracerProvider"/>.
/// </summary>
public static ITracerProvider GlobalInstance { get; protected set; } = new NoopTracerProvider();

/// <summary>
/// Returns an Tracer for a given name and version.
/// </summary>
/// <param name="name">Name of the instrumentation library.</param>
/// <param name="version">Version of the instrumentation library (optional).</param>
/// <returns>Tracer for the given name and version information.</returns>
public static Tracer GetTracer(string name, string version = null)
{
return GlobalInstance.GetTracer(name, version);
}

// for tests
internal void Reset()
{
GlobalInstance = new NoopTracerProvider();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public Task StartAsync(CancellationToken cancellationToken)
{
// Ensure the factory was created when the app starts.
// This will create and start any configured adapters.
this.serviceProvider.GetRequiredService<TracerFactoryBase>();
this.serviceProvider.GetRequiredService<ITracerProvider>();

return Task.CompletedTask;
}
Expand Down
Loading

0 comments on commit f700828

Please sign in to comment.