Merge remote-tracking branch 'upstream/master' into johananl-johananl…

…/span-interfaces

.github/workflows/protogen.yml

@@ -14,8 +14,8 @@ jobs:
     - uses: actions/setup-go@v2
       with:
         go-version: '^1.14.0'
-    - run: sudo apt-get -y install pax
-    - run: make -f Makefile.proto protobuf
+    - run: sudo apt-get -y install rsync wget unzip
+    - run: make -f Makefile.proto protobuf clean
     - uses: stefanzweifel/git-auto-commit-action@v4
       id: commit-changes
       with:

CHANGELOG.md

@@ -10,11 +10,14 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ### Added
 
+- `trace.WithIDGenerator()` `TracerProviderOption`. (#1363)
 - Add the `ReadOnlySpan` and `ReadWriteSpan` interfaces to provide better control for accessing span data. (#1360)
 
 ### Changed
 
+- Zipkin exporter relies on the status code for success rather than body read but still read the response body. (#1328)
 - Move the OpenCensus example into `example` directory. (#1359)
+- Moved the SDK's `internal.IDGenerator` interface in to the `sdk/trace` package to enable support for externally-defined ID generators. (#1363)
 - `NewExporter` and `Start` functions in `go.opentelemetry.io/otel/exporters/otlp` now receive `context.Context` as a first parameter. (#1357)
 - Rename `export.SpanData` to `export.SpanSnapshot` and use it only for exporting spans. (#1360)
 - Store the parent's full `SpanContext` rather than just its span ID in the `span` struct. (#1360)
@@ -24,6 +27,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 - Remove `errUninitializedSpan` as its only usage is now obsolete. (#1360)
 
+### Fixed
+
+- Metric SDK `SumObserver` and `UpDownSumObserver` instruments correctness fixes. (#1381)
+
 ## [0.14.0] - 2020-11-19
 
 ### Added

Makefile.proto

@@ -13,60 +13,117 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
-# This Makefile.proto has rules to generate *.pb.go files in
-# `exporters/otlp/internal/opentelemetry-proto-gen` from the .proto files in
-# `exporters/otlp/internal/opentelemetry-proto` using protoc with a go plugin.
+# This Makefile.proto has rules to generate go code for otlp
+# exporter. It does it by copying the proto files from
+# `exporters/otlp/internal/opentelemetry-proto` (which is a
+# submodule that needs to be checked out) into `gen/proto`, changing
+# the go_package option to a valid string, generating the go files and
+# finally copying the files into the module. The files are not
+# generated in place, because protoc generates a too-deep directory
+# structure.
 #
-# The protoc binary and other tools are sourced from a docker image
-# `PROTOC_IMAGE`.
+# Currently, all the generated code is in
+# `exporters/otlp/internal/opentelemetry-proto-gen`.
 #
-# Prereqs: The archiving utility `pax` is installed.
+# Prereqs: wget (for downloading the zip file with protoc binary),
+# unzip (for unpacking the archive), rsync (for copying back the
+# generated files).
 
-PROTOC_IMAGE          := namely/protoc-all:1.29_2
-PROTOBUF_VERSION      := v1
-OTEL_PROTO_SUBMODULE  := exporters/otlp/internal/opentelemetry-proto
-PROTOBUF_GEN_DIR      := exporters/otlp/internal/opentelemetry-proto-gen
-PROTOBUF_TEMP_DIR     := gen/pb-go
-PROTO_SOURCE_DIR      := gen/proto
-SUBMODULE_PROTO_FILES := $(wildcard $(OTEL_PROTO_SUBMODULE)/opentelemetry/proto/*/$(PROTOBUF_VERSION)/*.proto \
-                           $(OTEL_PROTO_SUBMODULE)/opentelemetry/proto/collector/*/$(PROTOBUF_VERSION)/*.proto)
-SOURCE_PROTO_FILES    := $(subst $(OTEL_PROTO_SUBMODULE),$(PROTO_SOURCE_DIR),$(SUBMODULE_PROTO_FILES))
+PROTOC_VERSION := 3.14.0
 
-default: protobuf
+TOOLS_DIR                       := $(abspath ./.tools)
+TOOLS_MOD_DIR                   := ./internal/tools
+PROTOBUF_VERSION                := v1
+OTEL_PROTO_SUBMODULE            := exporters/otlp/internal/opentelemetry-proto
+GEN_TEMP_DIR                    := gen
+SUBMODULE_PROTO_FILES           := $(wildcard $(OTEL_PROTO_SUBMODULE)/opentelemetry/proto/*/$(PROTOBUF_VERSION)/*.proto) $(wildcard $(OTEL_PROTO_SUBMODULE)/opentelemetry/proto/collector/*/$(PROTOBUF_VERSION)/*.proto)
 
-.PHONY: protobuf protobuf-source gen-protobuf copy-protobufs
-protobuf: protobuf-source gen-protobuf copy-protobufs
+ifeq ($(strip $(SUBMODULE_PROTO_FILES)),)
+$(error Submodule at $(OTEL_PROTO_SUBMODULE) is not checked out, use "git submodule update --init")
+endif
 
-protobuf-source: $(SOURCE_PROTO_FILES) | $(PROTO_SOURCE_DIR)/
+PROTOBUF_GEN_DIR   := exporters/otlp/internal/opentelemetry-proto-gen
+PROTOBUF_TEMP_DIR  := $(GEN_TEMP_DIR)/pb-go
+PROTO_SOURCE_DIR   := $(GEN_TEMP_DIR)/proto
+SOURCE_PROTO_FILES := $(subst $(OTEL_PROTO_SUBMODULE),$(PROTO_SOURCE_DIR),$(SUBMODULE_PROTO_FILES))
 
-# Changes go_package in .proto file to point to repo-local location
-define exec-replace-pkgname
-sed  's,go_package = "github.com/open-telemetry/opentelemetry-proto/gen/go,go_package = "go.opentelemetry.io/otel/exporters/otlp/internal/opentelemetry-proto-gen,' < $(1) > $(2)
+.DEFAULT_GOAL := protobuf
 
-endef
+UNAME_S := $(shell uname -s)
+UNAME_M := $(shell uname -m)
+
+ifeq ($(UNAME_S),Linux)
+
+PROTOC_OS := linux
+PROTOC_ARCH := $(UNAME_M)
+
+else ifeq ($(UNAME_S),Darwin)
+
+PROTOC_OS := osx
+PROTOC_ARCH := x86_64
+
+endif
 
-# replace opentelemetry-proto package name by go.opentelemetry.io/otel specific version
-$(SOURCE_PROTO_FILES): $(PROTO_SOURCE_DIR)/%.proto: $(OTEL_PROTO_SUBMODULE)/%.proto
-	@mkdir -p $(@D)
-	$(call exec-replace-pkgname,$<,$@)
+PROTOC_ZIP_URL := https://github.com/protocolbuffers/protobuf/releases/download/v$(PROTOC_VERSION)/protoc-$(PROTOC_VERSION)-$(PROTOC_OS)-$(PROTOC_ARCH).zip
 
-# Command to run protoc using docker image
-define exec-protoc-all
-docker run -v `pwd`:/defs $(PROTOC_IMAGE) $(1)
+$(TOOLS_DIR)/PROTOC_$(PROTOC_VERSION):
+	@rm -f "$(TOOLS_DIR)"/PROTOC_* && \
+	touch "$@"
 
+# Depend on a versioned file (like PROTOC_3.14.0), so when version
+# gets bumped, we will depend on a nonexistent file and thus download
+# a newer version.
+$(TOOLS_DIR)/protoc/bin/protoc: $(TOOLS_DIR)/PROTOC_$(PROTOC_VERSION)
+	echo "Fetching protoc $(PROTOC_VERSION)" && \
+	rm -rf $(TOOLS_DIR)/protoc && \
+	wget -O $(TOOLS_DIR)/protoc.zip $(PROTOC_ZIP_URL) && \
+	unzip $(TOOLS_DIR)/protoc.zip -d $(TOOLS_DIR)/protoc-tmp && \
+	rm $(TOOLS_DIR)/protoc.zip && \
+	touch $(TOOLS_DIR)/protoc-tmp/bin/protoc && \
+	mv $(TOOLS_DIR)/protoc-tmp $(TOOLS_DIR)/protoc
+
+$(TOOLS_DIR)/protoc-gen-gogofast: $(TOOLS_MOD_DIR)/go.mod $(TOOLS_MOD_DIR)/go.sum $(TOOLS_MOD_DIR)/tools.go
+	cd $(TOOLS_MOD_DIR) && \
+	go build -o $(TOOLS_DIR)/protoc-gen-gogofast github.com/gogo/protobuf/protoc-gen-gogofast && \
+	go mod tidy
+
+# Return a sed expression for replacing the go_package option in proto
+# file with a one that's valid for us.
+#
+# Example: $(call get-sed-expr,$(PROTOBUF_GEN_DIR))
+define get-sed-expr
+'s,go_package = "github.com/open-telemetry/opentelemetry-proto/gen/go,go_package = "go.opentelemetry.io/otel/$(1),'
 endef
 
-gen-protobuf: $(SOURCE_PROTO_FILES) | $(PROTOBUF_GEN_DIR)/
-	$(foreach file,$(subst ${PROTO_SOURCE_DIR}/,,$(SOURCE_PROTO_FILES)),$(call exec-protoc-all, -i $(PROTO_SOURCE_DIR) -f ${file} -l gogo -o ${PROTOBUF_TEMP_DIR}))
+.PHONY: protobuf
+protobuf: protobuf-source gen-protobuf copy-protobufs
+
+.PHONY: protobuf-source
+protobuf-source: $(SOURCE_PROTO_FILES)
+
+# This copies proto files from submodule into $(PROTO_SOURCE_DIR),
+# thus satisfying the $(SOURCE_PROTO_FILES) prerequisite. The copies
+# have their package name replaced by go.opentelemetry.io/otel.
+$(PROTO_SOURCE_DIR)/%.proto: $(OTEL_PROTO_SUBMODULE)/%.proto
+	@ \
+	mkdir -p $(@D); \
+	sed -e $(call get-sed-expr,$(PROTOBUF_GEN_DIR)) "$<" >"$@.tmp"; \
+	mv "$@.tmp" "$@"
 
-# requires `pax` to be installed, as it has consistent options for both BSD (Darwin) and Linux
-copy-protobufs: | $(PROTOBUF_GEN_DIR)/
-	find ./$(PROTOBUF_TEMP_DIR)/go.opentelemetry.io/otel/$(PROTOBUF_GEN_DIR) -type f -print0 | \
-      pax -0 -s ',^./$(PROTOBUF_TEMP_DIR)/go.opentelemetry.io/otel/$(PROTOBUF_GEN_DIR),,' -rw ./$(PROTOBUF_GEN_DIR)
+.PHONY: gen-protobuf
+gen-protobuf: $(SOURCE_PROTO_FILES) $(TOOLS_DIR)/protoc-gen-gogofast $(TOOLS_DIR)/protoc/bin/protoc
+	@ \
+	mkdir -p "$(PROTOBUF_TEMP_DIR)"; \
+	set -e; for f in $^; do \
+	  if [[ "$${f}" == $(TOOLS_DIR)/* ]]; then continue; fi; \
+	  echo "protoc $${f#"$(PROTO_SOURCE_DIR)/"}"; \
+	  PATH="$(TOOLS_DIR):$${PATH}" $(TOOLS_DIR)/protoc/bin/protoc --proto_path="$(PROTO_SOURCE_DIR)" --gogofast_out="plugins=grpc:$(PROTOBUF_TEMP_DIR)" "$${f}"; \
+	done
 
-$(PROTO_SOURCE_DIR)/ $(PROTOBUF_GEN_DIR)/:
-	mkdir -p $@
+.PHONY: copy-protobufs
+copy-protobufs:
+	@rsync -a $(PROTOBUF_TEMP_DIR)/go.opentelemetry.io/otel/exporters .
 
 .PHONY: clean
 clean:
-	rm -rf ./gen
+	rm -rf $(GEN_TEMP_DIR)

exporters/trace/zipkin/zipkin.go

@@ -20,6 +20,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"log"
 	"net/http"
@@ -112,14 +113,14 @@ func NewRawExporter(collectorURL, serviceName string, opts ...Option) (*Exporter
 // NewExportPipeline sets up a complete export pipeline
 // with the recommended setup for trace provider
 func NewExportPipeline(collectorURL, serviceName string, opts ...Option) (*sdktrace.TracerProvider, error) {
-	exp, err := NewRawExporter(collectorURL, serviceName, opts...)
+	exporter, err := NewRawExporter(collectorURL, serviceName, opts...)
 	if err != nil {
 		return nil, err
 	}
 
-	tp := sdktrace.NewTracerProvider(sdktrace.WithBatcher(exp))
-	if exp.o.config != nil {
-		tp.ApplyConfig(*exp.o.config)
+	tp := sdktrace.NewTracerProvider(sdktrace.WithBatcher(exporter))
+	if exporter.o.config != nil {
+		tp.ApplyConfig(*exporter.o.config)
 	}
 
 	return tp, err
@@ -166,19 +167,21 @@ func (e *Exporter) ExportSpans(ctx context.Context, ss []*export.SpanSnapshot) e
 	if err != nil {
 		return e.errf("request to %s failed: %v", e.url, err)
 	}
-	e.logf("zipkin responded with status %d", resp.StatusCode)
+	defer resp.Body.Close()
 
-	_, err = ioutil.ReadAll(resp.Body)
+	// Zipkin API returns a 202 on success and the content of the body isn't interesting
+	// but it is still being read because according to https://golang.org/pkg/net/http/#Response
+	// > The default HTTP client's Transport may not reuse HTTP/1.x "keep-alive" TCP connections
+	// > if the Body is not read to completion and closed.
+	_, err = io.Copy(ioutil.Discard, resp.Body)
 	if err != nil {
-		// Best effort to clean up here.
-		resp.Body.Close()
 		return e.errf("failed to read response body: %v", err)
 	}
 
-	err = resp.Body.Close()
-	if err != nil {
-		return e.errf("failed to close response body: %v", err)
+	if resp.StatusCode != http.StatusAccepted {
+		return e.errf("failed to send spans to zipkin server with status %d", resp.StatusCode)
 	}
+
 	return nil
 }
 

exporters/trace/zipkin/zipkin_test.go

@@ -192,6 +192,7 @@ func (c *mockZipkinCollector) handler(w http.ResponseWriter, r *http.Request) {
 	c.lock.Lock()
 	defer c.lock.Unlock()
 	c.models = append(c.models, models...)
+	w.WriteHeader(http.StatusAccepted)
 }
 
 func (c *mockZipkinCollector) Close() {
@@ -340,17 +341,15 @@ func TestExportSpans(t *testing.T) {
 	ctx := context.Background()
 	require.Len(t, ls.Messages, 0)
 	require.NoError(t, exporter.ExportSpans(ctx, spans[0:1]))
-	require.Len(t, ls.Messages, 2)
+	require.Len(t, ls.Messages, 1)
 	require.Contains(t, ls.Messages[0], "send a POST request")
-	require.Contains(t, ls.Messages[1], "zipkin responded")
 	ls.Messages = nil
 	require.NoError(t, exporter.ExportSpans(ctx, nil))
 	require.Len(t, ls.Messages, 1)
 	require.Contains(t, ls.Messages[0], "no spans to export")
 	ls.Messages = nil
 	require.NoError(t, exporter.ExportSpans(ctx, spans[1:2]))
 	require.Contains(t, ls.Messages[0], "send a POST request")
-	require.Contains(t, ls.Messages[1], "zipkin responded")
 	checkFunc := func() bool {
 		return collector.ModelsLen() == len(models)
 	}

internal/tools/go.mod

@@ -4,6 +4,7 @@ go 1.14
 
 require (
 	github.com/client9/misspell v0.3.4
+	github.com/gogo/protobuf v1.3.1
 	github.com/golangci/golangci-lint v1.33.0
 	github.com/itchyny/gojq v0.11.2
 	golang.org/x/tools v0.0.0-20201013201025-64a9e34f3752

internal/tools/go.sum

@@ -103,6 +103,8 @@ github.com/gofrs/flock v0.8.0/go.mod h1:F1TvTiK9OcQqauNUHlbJvyl9Qa1QvF/gOUDKA14j
 github.com/gogo/protobuf v1.1.1/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ=
 github.com/gogo/protobuf v1.2.1 h1:/s5zKNz0uPFCZ5hddgPdo2TK2TVrUNMn0OOX8/aZMTE=
 github.com/gogo/protobuf v1.2.1/go.mod h1:hp+jE20tsWTFYpLwKvXlhS1hjn+gTNwPg2I6zVXpSg4=
+github.com/gogo/protobuf v1.3.1 h1:DqDEcV5aeaTmdFBePNpYsp3FlcVH/2ISVVM9Qf8PSls=
+github.com/gogo/protobuf v1.3.1/go.mod h1:SlYgWuQ5SjCEi6WLHjHCa1yvBfUnHcTbrrZtXPKa29o=
 github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfUlaOkMMuAQtPIUF8ecWP5IEl/CR7VP2Q=
 github.com/golang/groupcache v0.0.0-20190129154638-5b532d6fd5ef/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc=
 github.com/golang/mock v1.1.1/go.mod h1:oTYuIxOrZwtPieC+H1uAHpcLFnEyAGVDL/k47Jfbm0A=
@@ -229,6 +231,7 @@ github.com/jtolds/gls v4.20.0+incompatible h1:xdiiI2gbIgH/gLH7ADydsJ1uDOEzR8yvV7
 github.com/jtolds/gls v4.20.0+incompatible/go.mod h1:QJZ7F/aHp+rZTRtaJ1ow/lLfFfVYBRgL+9YlvaHOwJU=
 github.com/julienschmidt/httprouter v1.2.0/go.mod h1:SYymIcj16QtmaHHD7aYtjjsJG7VTCxuUUipMqKk8s4w=
 github.com/kisielk/errcheck v1.1.0/go.mod h1:EZBBE59ingxPouuu3KfxchcWSUPOHkagtvWXihfKN4Q=
+github.com/kisielk/errcheck v1.2.0/go.mod h1:/BMXB+zMLi60iA8Vv6Ksmxu/1UDYcXs4uQLJ+jE2L00=
 github.com/kisielk/gotool v1.0.0 h1:AV2c/EiW3KqPNT9ZKl07ehoAGi4C5/01Cfbblndcapg=
 github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck=
 github.com/klauspost/compress v1.10.7/go.mod h1:aoV0uJVorq1K+umq18yTdKaF57EivdYsUV+/s2qKfXs=
@@ -545,6 +548,7 @@ golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxb
 golang.org/x/tools v0.0.0-20180221164845-07fd8470d635/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
 golang.org/x/tools v0.0.0-20180525024113-a5b4c53f6e8b/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
 golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
+golang.org/x/tools v0.0.0-20181030221726-6c7e314b6563/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
 golang.org/x/tools v0.0.0-20190110163146-51295c7ec13a/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
 golang.org/x/tools v0.0.0-20190114222345-bf090417da8b/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
 golang.org/x/tools v0.0.0-20190221204921-83362c3779f5/go.mod h1:9Yl7xja0Znq3iFh3HoIrodX9oNMXvdceNzlUR8zjMvY=

internal/tools/tools.go

@@ -21,4 +21,5 @@ import (
 	_ "github.com/golangci/golangci-lint/cmd/golangci-lint"
 	_ "github.com/itchyny/gojq"
 	_ "golang.org/x/tools/cmd/stringer"
+	_ "github.com/gogo/protobuf/protoc-gen-gogofast"
 )

sdk/export/metric/metric.go

@@ -174,6 +174,9 @@ type Aggregator interface {
 	//
 	// This call has no Context argument because it is expected to
 	// perform only computation.
+	//
+	// When called with a nil `destination`, this Aggregator is reset
+	// and the current value is discarded.
 	SynchronizedMove(destination Aggregator, descriptor *metric.Descriptor) error
 
 	// Merge combines the checkpointed state from the argument