census-instrumentation / opencensus-csharp

Distributed tracing and stats collecting framework
https://opencensus.io
Apache License 2.0
139 stars 32 forks source link

Proper pattern matching implementation for AttributeValue #97

Closed SergeyKanzhelev closed 5 years ago

SergeyKanzhelev commented 5 years ago

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: https://github.com/census-instrumentation/opencensus-csharp/blob/d087855fb93440f678009173d3f0f4392b58ce52/src/OpenCensus.Abstractions/Trace/Internal/AttributeValue%7BT%7D.cs#L120-L149

  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.

SergeyKanzhelev commented 5 years ago

This will be handled in OpenTelemetry project