census-instrumentation / opencensus-csharp Goto Github PK

C# and .NET programming styles do not typically involve interface-for-everything approach.

We should have a discussion about it in opencensus and during separation of API and Impl (#8) ensure that interfaces are not overused

The proposal is to implement an optional out parameter in StartScopedSpan API so you can use using {} pattern and do not need to get the current span inside the using block again. Other scenarios performance may also be improved

Currently we don't have metrics and resource specifications implemented:
https://github.com/census-instrumentation/opencensus-specs/blob/master/metrics/Metrics.md
https://github.com/census-instrumentation/opencensus-proto/blob/master/src/opencensus/proto/metrics/v1/metrics.proto

we can leverage all existing instrumentation with DiagnosticSource and auto-collect Asp.NET Core, Http, Sql, ServiceBus, EventHubs

In the java source version, the concrete classes are created by AutoValue which, from my limited understanding, does not expose the concrete types. If we want to preserve that intention, then I'd propose our concrete classes (like BucketBoundaries, should not be sealed, but rather be static with just the Create method, and have private sealed implementation classes.

If the intention is to expose these concrete types (which may result in folks programming to those rather than the intended interfaces) then that's fine too - but at that point we may want to consider if we want to expose interfaces at all.

hi, there.
I'm trying to use your library. Thank you for it.

So, I found some inconsistency in the implementation of exporters.
Namely, 'Sum' aggregation in case of Prometheus converted to 'gauge', in case of StackDriver to CUMULATIVE, which is 'counter'.

The fun is that neither of both is totally right.
'Sum' can represent either 'counter' or 'gauge'. For instance, when we count peers or users currently online it is very convenient to use 'Sum' and, in this case, it should be converted to 'gauge' by all exporters because value can go up and down
The second case, when we count the amount of data sent. When we have just buffer length it is very convenient to use 'Sum', but in this case, a value will only grow and measure should be converted to 'counter'

So, I would propose to add a second type for 'Sum' aggregation. first will be converted to 'gauge' second to 'counter'

The spec statement about implementing the Context concept is

Languages that already have this support, like Go (context.Context) or C# (ExecutionContext), MUST use the language supported generic context instead of building their own.

Is there more info about a planned design in this direction? ExecutionContext doesn't seem great for extension, but I might be missing something.

https://docs.microsoft.com/en-us/dotnet/api/system.threading.executioncontext?view=netcore-2.0

subj

1. we should understand the performance of OC and fix bottlenecks if any
2. we should make it easy to run and experiment (e.g. opencensus-java allows to run benchmarking as unit tests) - can we do it?

Eliminate this hardcode in auto-collector:

I suggest as an initial implementation just have a custom sampler that will work per-request object before the main sampler like suggested here: https://github.com/rakyll/opencensus-specs/blob/7ae3810847d951ae79d0576afc3fd2cf0e020862/trace/HTTP.md#sampling

@simonz130

1. In SD trace exporter in all the exported spans add an extra attribute:
   String AGENT_LABEL_KEY = "g.co/agent";
   String AGENT_LABEL_VALUE_STRING =
   "opencensus-csharp %LIB_VERSION%; stackdriver-exporter %EXPORTER_VERSION%"
2. In SD monitoring exporter add the user-agent with the request:
   String USER_AGENT =
   "opencensus-csharp/%LIB_VERSION%"

There are only a few aggregation types currently supported by Prometheus exporter. Need to finalize all of them.

See

Support statsZ page

Implement exporter to local forwarder (grpc, using https://github.com/census-instrumentation/opencensus-proto/blob/master/opencensus/proto/exporter/exporter.proto)

Clean up time-related contracts. Most of them came as a port from Java SDK. After reviewing it I think we need the following:

• Timestamp class with nanosecond resolution. Make it struct. It should only used in SpanData as this resolution it only needed to be able to represent high-resolution clock - when you are importing data for instance, not to actually measure time in C# application. It may also be needed for stats Views, need more investigations.
• IClock should be removed as it is only used to abstract away getNanos system call in Java. The only promise of IClock is to give the start time of a span and DateTimeOffset.Now can do it well.
• TimesampConverter is needed as it is used to calculate latency with high precision using StopWatch from the startTime of the outermost span. All children spans and time events should the same instance of TimestampConverter to ensure the durations and position of time events is monotonic and precise. Theoretically we can use Stopwatch class instead of using this wrapper. The only benefit of a wrapper is that it will encapsulate both - start time and stopwatch to generate high-precision start time.

Motivation: Exposed API
User impact: Confusion

The Value property on an instance of CurrentStatsState exposes an empty set method. Either

1. That property setter should call Set(StatsCollectionState state)
2. That property setter should be removed
3. That property setter should have modified visibility to hide it from consumers

tracestate may carry sampling-related information. So ShouldSample method of ISampler must take it as an argument

Code from SteelToe will be ported to this repository: https://github.com/SteeltoeOSS/Management/tree/dev/src/Steeltoe.Management.OpenCensus

You can start looking into code and plan for contributions

Hi!
Is Jaeger exporter considered?

This is the issue to collect feedback on releases schedule for this repository. Current thinking is to schedule releases like one beta every month and stable version twice a year. Basically calendar-based, not feature based.

We will also align marking SDK 1.0 with other repositories.

As described in #92 - there are cases when you might need to report a span from the past. In that PR Redis library calls are recorded asynchronously. My guess why this API was chosen to expose Redis calls is extreme care of those calls latency.

Current API is quite limiting in this sense.

1. ISpan doesn't allow to set start and end timestamps explicitly. Same for time events like annotations. Expanding ISpan interface may create some confusions.
2. Alternative is to report SpanData directly. This approach can be problematic as a lot of logic like calling start/stop handler and sampling needs to be replicated manually.
3. Another alternative is to either create custom Redis implementation of ISpan or make ISpan constructible from SpanData. I like this last approach. Not sure what may be the problems here.

Mostly for @lmolkova (I think). I found this doc while looking for something else https://docs.microsoft.com/en-us/dotnet/framework/debug-trace-profile/tracing-and-instrumenting-applications and it looks like it's saying for dotnet to use Trace and doesn't mention Activity. There may be reasons for this I just wanted to put it on your radar.

https://github.com/w3c/distributed-tracing

ByteToHexCharArray seems to be extremely inefficient with all allocations it does. Here is a wonderful perf analysis comparison for different algos to convert byte array to string https://stackoverflow.com/questions/311165/how-do-you-convert-a-byte-array-to-a-hexadecimal-string-and-vice-versa

we should do this: https://stackoverflow.com/questions/311165/how-do-you-convert-a-byte-array-to-a-hexadecimal-string-and-vice-versa/24343727#24343727

• Namespaces should not contain vendor name and should be aligned with Java OC
• unused usings
• commented code
• remove AsyncLocal OnChange subscription
• code style checks
• latest C# features: expression-bodied props, etc...
• empty lines
• sln project
• gitignore
• readme
• making the enums pascal cased

Check ocagent exporter: it has to convert any attribute to string because there is no way to check value type:

Check how it's done for protobuf oneof case: https://github.com/Microsoft/ApplicationInsights-LocalForwarder/blob/master/src/Library/OpenCensusTelemetryConverter.cs#L527

Currently opencensus targets 4.6 and netstandard 2.0.
.NET 4.5 is not officially supported by Microsoft, but still is very popular.

Caveat: AsyncLocal is not available on .NET 4.5.

Here is the code that System.Diagnostics.Activity uses instead. Note what it does to avoid cross AppDomain calls issues.

Reviewing API surface - IList is used when IEnumerable will suffice. For instance in Exporter handler and other places. IList allows manipulating elements - add, remove, etc. when this is absolutely not the intent.

In some environments opening separate port for Prometheus exporter will not work. Especially for sandboxed environments like Azure Web Apps. Re-using app's port and allow Prometheus to work as a middleware will help to enable those environments.

So that library can take a dependency on API only allowing multiple implementations and more flexibility on impl side

I am assuming this should work but it doesnt listen on anything related to port 9184 then, if I change it to localhost instead of 0.0.0.0 then it works. Is it a bug or intentional?

var exporter = new PrometheusExporter(
new PrometheusExporterOptions()
{
    Url = new Uri("http://0.0.0.0:9184/metrics/")
},
Stats.ViewManager);

exporter.Start();

Hi, is there a timeline for support for EF core and psql/mysql/mssql?

Motivation: Performance
User impact: None/Hypothetical

Oops, that wasn't meant to make it in as-is, mostly just to demonstrate the problem with the test :)

RE: #29, a lock is being taken for every read. We should investigate alternative methods as the type has the concurrency properties of being unlikely to be written to frequently (in fact, it's disallowed) and being likely to be read more than it is written. Perhaps ReaderWriterLock would be sufficient here.

Span.Kind should be set early to allow correct metrics calculation. This should be reflected in API design - getter/setter pattern may not be ideal here. Consider requiring span Kind in constructor or Create method.

Hi,

Thank you for working on this project. I tried to get latest beta build from MyGet today (i am particularly interested in #109) and realized that CI is red from the end of January.

Failing test is OpenCensus.Exporter.ApplicationInsights.Tests.OpenCensusTelemetryConverterTests.OpenCensusTelemetryConverterTests_TracksRequestWithAnnotations which fails on Assert.NonEqual. I do not have enough knowledge to understand why it fails.

Thanks.

This repository uses pattern matching with some errors:

The purpose of Match method is to avoid if/else checks by supplying set of callbacks to call. And each implementation calls respective callback. Something like a virtual methods in generic class, but strongly typed for pre-defined types.

Issues:

1. AttributeValue uses if/else instead of having separate classes with the hardcoded implementation of a Match method which will call the proper callback:
   

   opencensus-csharp/src/OpenCensus.Abstractions/Trace/Internal/AttributeValue{T}.cs

   Lines 120 to 149 in d087855

   	public override TReturn Match<TReturn>(
   	Func<string, TReturn> stringFunction,
   	Func<bool, TReturn> booleanFunction,
   	Func<long, TReturn> longFunction,
   	Func<double, TReturn> doubleFunction,
   	Func<object, TReturn> defaultFunction)
   	{
   	if (typeof(T) == typeof(string))
   	{
   	string value = this.Value as string;
   	return stringFunction(value);
   	}
   	else if (typeof(T) == typeof(long))
   	{
   	long val = (long)(object)this.Value;
   	return longFunction(val);
   	}
   	else if (typeof(T) == typeof(bool))
   	{
   	bool val = (bool)(object)this.Value;
   	return booleanFunction(val);
   	}
   	else if (typeof(T) == typeof(double))
   	{
   	double val = (double)(object)this.Value;
   	return doubleFunction(val);
   	}
   	return defaultFunction(this.Value);
   	}

2. There is no need to expose type-specific results in factory methods AttributeValue.StringAttributeValue. Simply returning IAttributeValue should suffice. So generic IAttributeValue<T> interface is not needed.

3. Implementation of IAttributeValue is trivial and there is no need to have interface and implementation separately - simple implementation class will suffice.

We typically log all internal errors/warning/traces to EventSource in other .NET libs.

We should have license for all classes. See example in other repos.

Let's try to start with CircleCI to align with other repos.

I've tried using distribution aggregation with stackdriver exporter, but it does not work.
It looks like the same issue as in GO package - census-instrumentation/opencensus-go#143

See motivation for Activity: https://github.com/dotnet/corefx/issues/29207

TL;DR;
There are certain cases when async context is not reliable. The most popular case is native code invocation which is common case in ASP.NET classic or libs like System.Drawing.

We should be able to provide users and integrations a way to restore context.

All exporter are expected to send data asynchronously, this method must be async

Can impl package depend on Ms.Extensions.DependencyInjection? It should be just fine for .NET Core apps.

In this case TraceComponent could be removed completely

Could they be removed at least from API package?

Support RPCz page to expose span buckets

Guid.NextGuid().GetBytes is a better alternative

'monitorring' is the tag instead of 'monitoring'

See comments here:

#41 (review)