Reorganize propagation code (shrink PR 381)

[krnowak]

This PR moves some propagation-related files around without the significant changes to the code (most of the diff is because of the package renames). This is to shrink #381. The propagation interfaces will be later reworked, but at least they will stay in the packages as proposed here.

The organization of the files proposed here is as follows:

• propagation interfaces definitions are in api/context/propagation
• propagation code specific to baggage is in api/context/baggage/bpropagation
  • api/context/baggage contains a definition of the map
• propagation code specific to tracing is in api/trace/tpropagation

The changes I have made (compared to #381) is that I have renamed the trace propagation package to tpropagation and baggage propagation package to bpropagation. This is to avoid name clashes on imports that would always be fixed by using a named import (like import traceprop "go.opentelemetry.io/api/trace/propagation").

There is one change I haven't made, but I'm still inclined to do is to remove the "context" package from api/context/* packages, so we would have api/propagation with propagation interfaces, api/baggage with the map and api/baggage/bpropagation with the baggage propagation code. What do you think?

[MrAlias]

The restructuring all looks good.

The package names tpropagation and bpropagation do not seem like good package names to me. The first letter coming from each of their containing directory seems like a stutter and a decision made based on another packages existence. The fact that the api/context/propagation package is also not named api/context/cpropagation seem to me like we are being inconsistent.

I'm in favor of just calling all the packages propagation and scoping the package based on the directory they are in. Using the language's package renaming syntax to resolve the naming collisions I think is an acceptable approach. That is the reason it was included in the language.

That all being said, it seems to me that naming is not the main point of this PR. I'm fine with merging it as is if we want to address the package naming later. Or, if I'm the only one with this view, I'm fine with going along with this decision.

[krnowak]

The restructuring all looks good.

The package names tpropagation and bpropagation do not seem like good package names to me. The first letter coming from each of their containing directory seems like a stutter and a decision made based on another packages existence. The fact that the api/context/propagation package is also not named api/context/cpropagation seem to me like we are being inconsistent.

api/context/propagation is a base package, which is why I left it at propagation, not cpropagation. For tpropagation and bpropagation I kinda sorta followed the practice from golang standard library (they have io/ioutil, net/http/httputil). "util" is a very common name in golang standard library, just as "propagation" is a common name in opentelemetry-go. That's why the b and t before propagation. I could name the packages baggagepropagation and tracepropagation but that's quite a mouthful. It was a compromise between {baggage,trace}propagation and {b,t}prop. ;) Other possibilities are traceprop and baggageprop, but for me they are ugliest of them all.

I'm in favor of just calling all the packages propagation and scoping the package based on the directory they are in. Using the language's package renaming syntax to resolve the naming collisions I think is an acceptable approach. That is the reason it was included in the language.

I'd say the reason for it in the language is the inevitable existence of name clashes between various libraries imported in the same file. But within a library, I'd prefer to avoid having such clashes.

That all being said, it seems to me that naming is not the main point of this PR. I'm fine with merging it as is if we want to address the package naming later. Or, if I'm the only one with this view, I'm fine with going along with this decision.

Thanks for the review.

I'm still on the fence with the organization. Maybe api/context/propagation, api/baggage, api/baggage/bpropagation and api/trace/tpropagation would be a better idea (moving baggage out of api/context)? Also, not sure about the existence of api/context - it is going to be empty.

[jmacd]

I won't object if we change package structure, and I don't know what's best.
I think we shouldn't use "tpropagation" and "bpropagation".
There were other prototypes for OTEP 66 -- we coordinated on the paths named context/propagation, but it was confusing to have three packages named "propagation".

I can imagine the contents of api/trace/propagation could move into api/trace. The contents of api/context/baggage/propagation could move into api/correlation along with the baggage.Map type. Then api/context/propagation can become api/propagation.

[rghetia]

I can imagine the contents of api/trace/propagation could move into api/trace. The contents of api/context/baggage/propagation could move into api/correlation along with the baggage.Map type. Then api/context/propagation can become api/propagation.

+1

[krnowak]

I won't object if we change package structure, and I don't know what's best.
I think we shouldn't use "tpropagation" and "bpropagation".
There were other prototypes for OTEP 66 -- we coordinated on the paths named context/propagation, but it was confusing to have three packages named "propagation".

I can imagine the contents of api/trace/propagation could move into api/trace. The contents of api/context/baggage/propagation could move into api/correlation along with the baggage.Map type. Then api/context/propagation can become api/propagation.

Sounds good, thanks.

[krnowak]

Got an import cycle, yeeeey…

compiling all packages in .
# go.opentelemetry.io/otel/api/trace
import cycle not allowed in test
package go.opentelemetry.io/otel/api/trace (test)
	imports go.opentelemetry.io/otel/internal/trace
	imports go.opentelemetry.io/otel/api/trace
make: *** [Makefile:55: build] Error 1
[kv@localhost ([go-1.13, lightstep]) ctxprop2]$ git grep internal/trace
api/trace/b3_propagator_benchmark_test.go:      mocktrace "go.opentelemetry.io/otel/internal/trace"
api/trace/b3_propagator_test.go:        mocktrace "go.opentelemetry.io/otel/internal/trace"
api/trace/trace_context_propagator_benchmark_test.go:   mocktrace "go.opentelemetry.io/otel/internal/trace"
api/trace/trace_context_propagator_test.go:     mocktrace "go.opentelemetry.io/otel/internal/trace"
plugin/othttp/handler_test.go:  mocktrace "go.opentelemetry.io/otel/internal/trace"

Will move the tests into a api/trace/tests directory to avoid the cycle. This unfortunately breaks the coverage reports, but since we have none, I'll punt the problem until later.

[krnowak]

Updated. I moved the trace propagators test to the already existing api/trace/tracetest package. Also, now we have trace.TraceContext about which golint complains about stutter, so I shut it up.

[rghetia]

Updated. I moved the trace propagators test to the already existing api/trace/tracetest package. Also, now we have trace.TraceContext about which golint complains about stutter, so I shut it up.

How about renaming it trace.ContextPropagator?

[krnowak]

Updated. I moved the trace propagators test to the already existing api/trace/tracetest package. Also, now we have trace.TraceContext about which golint complains about stutter, so I shut it up.

How about renaming it trace.ContextPropagator?

I'm reluctant. TraceContext is a name of the W3C spec that this propagator implements. It also sounds too generic - might give an idea that this is the propagator you want to use (while there is also b3 propagator to choose from).

I was rather thinking about keeping the name and adding the Propagator part, so it is consistent with trace.B3Propagator.

But I'd prefer to just move on now and I could file an issue about names after this PR gets merged. WDYT?

[rghetia]

I was rather thinking about keeping the name and adding the Propagator part, so it is consistent with trace.B3Propagator.

Long, but I don't have better suggestion.

But I'd prefer to just move on now and I could file an issue about names after this PR gets merged. WDYT?

sure. Opened #448