From a12224a454135a5d7ec17831b6b39cf9723c0cdb Mon Sep 17 00:00:00 2001
From: Matej Gera <38492574+matej-g@users.noreply.github.com>
Date: Wed, 16 Sep 2020 18:09:45 +0200
Subject: [PATCH] Ensure exported interfaces have named method parameters
 (#1172)

* Ensure exported interface types have named arguments

* Update Styling Guide

* update CHANGELOG
---
 CHANGELOG.md                            |  2 ++
 CONTRIBUTING.md                         |  8 ++++++++
 api/metric/metrictest/async.go          |  4 ++--
 api/metric/sdkapi.go                    |  6 +++---
 api/propagation/propagation.go          |  4 ++--
 api/trace/tracetest/config.go           |  4 ++--
 bridge/opentracing/migration/api.go     |  2 +-
 label/encoder.go                        |  2 +-
 sdk/export/metric/metric.go             | 14 +++++++-------
 sdk/export/trace/trace.go               |  4 ++--
 sdk/metric/controller/time/time.go      |  2 +-
 sdk/metric/processor/reducer/reducer.go |  2 +-
 sdk/trace/sampling.go                   |  2 +-
 13 files changed, 33 insertions(+), 23 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 53e71f386..3ccaf077a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -39,6 +39,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - The [configuration style guide](https://github.com/open-telemetry/opentelemetry-go/blob/master/CONTRIBUTING.md#config) has been updated to
    recommend the use of `newConfig()` instead of `configure()`. (#1163)
 - The `otlp.Config` type has been unexported and changed to `otlp.config`, along with its initializer. (#1163)
+- Ensure exported interface types include parameter names and update the
+   Style Guide to reflect this styling rule. (#1172)
 - Don't consider unset environment variable for resource detection to be an error. (#1170)
 - Rename `go.opentelemetry.io/otel/api/metric.ConfigureInstrument` to `NewInstrumentConfig` and
   `go.opentelemetry.io/otel/api/metric.ConfigureMeter` to `NewMeterConfig`.
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 5c06abb07..85d9a0987 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -345,6 +345,14 @@ func NewDog(name string, o ...DogOption) Dog    {…}
 func NewBird(name string, o ...BirdOption) Bird {…}
 ```
 
+### Interface Type
+
+To allow other developers to better comprehend the code, it is important
+to ensure it is sufficiently documented. One simple measure that contributes
+to this aim is self-documenting by naming method parameters. Therefore,
+where appropriate, methods of every exported interface type should have
+their parameters appropriately named.
+
 ## Approvers and Maintainers
 
 Approvers:
diff --git a/api/metric/metrictest/async.go b/api/metric/metrictest/async.go
index bc8a7d557..cd908f2e2 100644
--- a/api/metric/metrictest/async.go
+++ b/api/metric/metrictest/async.go
@@ -32,7 +32,7 @@ var ErrInvalidAsyncRunner = errors.New("unknown async runner type")
 // the SDK to provide support for running observer callbacks.
 type AsyncCollector interface {
 	// CollectAsync passes a batch of observations to the MeterImpl.
-	CollectAsync([]label.KeyValue, ...metric.Observation)
+	CollectAsync(labels []label.KeyValue, observation ...metric.Observation)
 }
 
 // AsyncInstrumentState manages an ordered set of asynchronous
@@ -49,7 +49,7 @@ type AsyncInstrumentState struct {
 	// collection interval.  Singletons are entered with a real
 	// instrument each, batch observers are entered with a nil
 	// instrument, ensuring that when a singleton callback is used
-	// repeatedly, it is excuted repeatedly in the interval, while
+	// repeatedly, it is executed repeatedly in the interval, while
 	// when a batch callback is used repeatedly, it only executes
 	// once per interval.
 	runnerMap map[asyncRunnerPair]struct{}
diff --git a/api/metric/sdkapi.go b/api/metric/sdkapi.go
index 20c390560..122c9ba6d 100644
--- a/api/metric/sdkapi.go
+++ b/api/metric/sdkapi.go
@@ -24,7 +24,7 @@ import (
 // implementation.
 type MeterImpl interface {
 	// RecordBatch atomically records a batch of measurements.
-	RecordBatch(context.Context, []label.KeyValue, ...Measurement)
+	RecordBatch(ctx context.Context, labels []label.KeyValue, measurement ...Measurement)
 
 	// NewSyncInstrument returns a newly constructed
 	// synchronous instrument implementation or an error, should
@@ -85,10 +85,10 @@ type AsyncImpl interface {
 
 // WrapMeterImpl constructs a `Meter` implementation from a
 // `MeterImpl` implementation.
-func WrapMeterImpl(impl MeterImpl, instrumentatioName string, opts ...MeterOption) Meter {
+func WrapMeterImpl(impl MeterImpl, instrumentationName string, opts ...MeterOption) Meter {
 	return Meter{
 		impl:    impl,
-		name:    instrumentatioName,
+		name:    instrumentationName,
 		version: NewMeterConfig(opts...).InstrumentationVersion,
 	}
 }
diff --git a/api/propagation/propagation.go b/api/propagation/propagation.go
index 8e2d38338..860fbcae6 100644
--- a/api/propagation/propagation.go
+++ b/api/propagation/propagation.go
@@ -43,7 +43,7 @@ type HTTPExtractor interface {
 	// trace.ContextWithRemoteSpanContext. In case of correlation
 	// context, the propagator should use correlation.WithMap to
 	// store it in the context.
-	Extract(context.Context, HTTPSupplier) context.Context
+	Extract(ctx context.Context, supplier HTTPSupplier) context.Context
 }
 
 // HTTPInjector injects information into a HTTPSupplier.
@@ -52,7 +52,7 @@ type HTTPInjector interface {
 	// encodes it into propagator specific format and then injects
 	// the encoded information using supplier into an associated
 	// carrier.
-	Inject(context.Context, HTTPSupplier)
+	Inject(ctx context.Context, supplier HTTPSupplier)
 }
 
 // Config contains the current set of extractors and injectors.
diff --git a/api/trace/tracetest/config.go b/api/trace/tracetest/config.go
index bb97266c7..caa79e897 100644
--- a/api/trace/tracetest/config.go
+++ b/api/trace/tracetest/config.go
@@ -89,8 +89,8 @@ func WithSpanRecorder(sr SpanRecorder) Option {
 }
 
 type SpanRecorder interface {
-	OnStart(*Span)
-	OnEnd(*Span)
+	OnStart(span *Span)
+	OnEnd(span *Span)
 }
 
 type StandardSpanRecorder struct {
diff --git a/bridge/opentracing/migration/api.go b/bridge/opentracing/migration/api.go
index e08689872..3b866b210 100644
--- a/bridge/opentracing/migration/api.go
+++ b/bridge/opentracing/migration/api.go
@@ -72,5 +72,5 @@ type OverrideTracerSpanExtension interface {
 	// API calls. In such case, there is no need to use the
 	// WrapperTracer and thus no need to override the result of
 	// the Tracer() function.
-	OverrideTracer(oteltrace.Tracer)
+	OverrideTracer(tracer oteltrace.Tracer)
 }
diff --git a/label/encoder.go b/label/encoder.go
index 17e6d2dfb..6be7a3f6b 100644
--- a/label/encoder.go
+++ b/label/encoder.go
@@ -29,7 +29,7 @@ type (
 		// Encode returns the serialized encoding of the label
 		// set using its Iterator.  This result may be cached
 		// by a label.Set.
-		Encode(Iterator) string
+		Encode(iterator Iterator) string
 
 		// ID returns a value that is unique for each class of
 		// label encoder.  Label encoders allocate these using
diff --git a/sdk/export/metric/metric.go b/sdk/export/metric/metric.go
index b0996a712..d47283b14 100644
--- a/sdk/export/metric/metric.go
+++ b/sdk/export/metric/metric.go
@@ -71,7 +71,7 @@ type Processor interface {
 	// computation.  An SDK is not expected to call exporters from
 	// with Process, use a controller for that (see
 	// ./controllers/{pull,push}.
-	Process(Accumulation) error
+	Process(accum Accumulation) error
 }
 
 // AggregatorSelector supports selecting the kind of Aggregator to
@@ -94,7 +94,7 @@ type AggregatorSelector interface {
 	// Note: This is context-free because the aggregator should
 	// not relate to the incoming context.  This call should not
 	// block.
-	AggregatorFor(*metric.Descriptor, ...*Aggregator)
+	AggregatorFor(descriptor *metric.Descriptor, aggregator ...*Aggregator)
 }
 
 // Checkpointer is the interface used by a Controller to coordinate
@@ -152,7 +152,7 @@ type Aggregator interface {
 	//
 	// The Context argument comes from user-level code and could be
 	// inspected for a `correlation.Map` or `trace.SpanContext`.
-	Update(context.Context, metric.Number, *metric.Descriptor) error
+	Update(ctx context.Context, number metric.Number, descriptor *metric.Descriptor) error
 
 	// SynchronizedMove is called during collection to finish one
 	// period of aggregation by atomically saving the
@@ -181,7 +181,7 @@ type Aggregator interface {
 	//
 	// The owner of an Aggregator being merged is responsible for
 	// synchronization of both Aggregator states.
-	Merge(Aggregator, *metric.Descriptor) error
+	Merge(aggregator Aggregator, descriptor *metric.Descriptor) error
 }
 
 // Subtractor is an optional interface implemented by some
@@ -206,7 +206,7 @@ type Exporter interface {
 	//
 	// The CheckpointSet interface refers to the Processor that just
 	// completed collection.
-	Export(context.Context, CheckpointSet) error
+	Export(ctx context.Context, checkpointSet CheckpointSet) error
 
 	// ExportKindSelector is an interface used by the Processor
 	// in deciding whether to compute Delta or Cumulative
@@ -221,7 +221,7 @@ type ExportKindSelector interface {
 	// ExportKindFor should return the correct ExportKind that
 	// should be used when exporting data for the given metric
 	// instrument and Aggregator kind.
-	ExportKindFor(*metric.Descriptor, aggregation.Kind) ExportKind
+	ExportKindFor(descriptor *metric.Descriptor, aggregatorKind aggregation.Kind) ExportKind
 }
 
 // CheckpointSet allows a controller to access a complete checkpoint of
@@ -242,7 +242,7 @@ type CheckpointSet interface {
 	// expected from the Meter implementation. Any other kind
 	// of error will immediately halt ForEach and return
 	// the error to the caller.
-	ForEach(ExportKindSelector, func(Record) error) error
+	ForEach(kindSelector ExportKindSelector, recordFunc func(Record) error) error
 
 	// Locker supports locking the checkpoint set.  Collection
 	// into the checkpoint set cannot take place (in case of a
diff --git a/sdk/export/trace/trace.go b/sdk/export/trace/trace.go
index 64935d042..370adb350 100644
--- a/sdk/export/trace/trace.go
+++ b/sdk/export/trace/trace.go
@@ -40,12 +40,12 @@ type SpanExporter interface {
 	// calls this function will not implement any retry logic. All errors
 	// returned by this function are considered unrecoverable and will be
 	// reported to a configured error Handler.
-	ExportSpans(context.Context, []*SpanData) error
+	ExportSpans(ctx context.Context, spanData []*SpanData) error
 	// Shutdown notifies the exporter of a pending halt to operations. The
 	// exporter is expected to preform any cleanup or synchronization it
 	// requires while honoring all timeouts and cancellations contained in
 	// the passed context.
-	Shutdown(context.Context) error
+	Shutdown(ctx context.Context) error
 }
 
 // SpanData contains all the information collected by a completed span.
diff --git a/sdk/metric/controller/time/time.go b/sdk/metric/controller/time/time.go
index 9d0e4eb79..08bc44dbf 100644
--- a/sdk/metric/controller/time/time.go
+++ b/sdk/metric/controller/time/time.go
@@ -24,7 +24,7 @@ import (
 
 type Clock interface {
 	Now() lib.Time
-	Ticker(lib.Duration) Ticker
+	Ticker(duration lib.Duration) Ticker
 }
 
 type Ticker interface {
diff --git a/sdk/metric/processor/reducer/reducer.go b/sdk/metric/processor/reducer/reducer.go
index aa901b4ea..c5fc5cec3 100644
--- a/sdk/metric/processor/reducer/reducer.go
+++ b/sdk/metric/processor/reducer/reducer.go
@@ -31,7 +31,7 @@ type (
 	// LabelFilterSelector is the interface used to configure a
 	// specific Filter to an instrument.
 	LabelFilterSelector interface {
-		LabelFilterFor(*metric.Descriptor) label.Filter
+		LabelFilterFor(descriptor *metric.Descriptor) label.Filter
 	}
 )
 
diff --git a/sdk/trace/sampling.go b/sdk/trace/sampling.go
index bf2e9c4be..84222983a 100644
--- a/sdk/trace/sampling.go
+++ b/sdk/trace/sampling.go
@@ -24,7 +24,7 @@ import (
 
 // Sampler decides whether a trace should be sampled and exported.
 type Sampler interface {
-	ShouldSample(SamplingParameters) SamplingResult
+	ShouldSample(parameters SamplingParameters) SamplingResult
 	Description() string
 }