Consolidate API surface area into a coherent package #295
Conversation
's:go.opentelemetry.io/otel/api/core:go.opentelemetry.io/otel:g' {} \;
This duplicated a bunch of code already in otel (previously api/core).
I removed the KeyValue constructors in favor of:
`otel.Key(<string>).ValueConstructor(<raw value)`.
So `apikey.Bool("key",true)` becomes `otel.Key("key").Bool(true)`.
freeformz
force-pushed
the
ffz/ConsolidateAPI
branch
from
75742cf to
338552e
Compare
I'm not really sure about this, but it seems like a better place than as a sub package of api.
Switching to `/usr/bin/env bash` allows for the script to pick up user installed versions of bash.
These docs should still be improved. There is likely an opportunity to rename some of the context functions a little as well.
The contents of this file have to do with the tracing aspects of otel.
There is still some work to do wrt renaming a few things.
These names just don't make sense to me (nor did their docs), but I feel like the new names make more sense.
freeformz
requested review from
bogdandrutu,
jmacd,
pjanotti,
rghetia,
sjkaris and
tedsuo
as code owners
|
Related to #261 |
ghost
mentioned this pull request
|
Do we also need to move otel/sdk/* in the future? |
There was a problem hiding this comment.
I did try to review this but gave up because there are a lot of things that needs to be renamed (I have made some comments that shows some examples) to have a clear and expressive API and I couldn't go through it all at once.
IMO, we should break this down in smaller PRs.
| @@ -91,10 +89,10 @@ func (s *bridgeSpan) Finish() { | |||
| } | |||
|
|
|||
| func (s *bridgeSpan) FinishWithOptions(opts ot.FinishOptions) { | |||
| var otelOpts []oteltrace.EndOption | |||
| var otelOpts []otel.EndOption | |||
There was a problem hiding this comment.
One of the problems that we get by merging all this packages is the mess with functional options. With your changes we someone is starting a span, finishing or doing another operation that accepts options how can one know what options are available to them?
Sure you can see the return type of each WithOption but it much better if we can decrease these options based on the operation. E.g., creating a span:
I don't know how this could be solved, would you consider creating traceopts, metricsopts and etc?
| @@ -12,7 +12,7 @@ | |||
| // See the License for the specific language governing permissions and | |||
| // limitations under the License. | |||
|
|
|||
| package core | |||
| package otel | |||
There was a problem hiding this comment.
We discussed a little at the Go SIG meeting that maybe KeyValue could go to it own package. It seems, by looking at the gitter chat, that people prefer a kv package, changing the UX to kv.String("key", "value") or "kv.Int("num", 1).
There was a problem hiding this comment.
+1 that kv.String(...) is more understandable.
| @@ -12,7 +12,7 @@ | |||
| // See the License for the specific language governing permissions and | |||
| // limitations under the License. | |||
|
|
|||
| package core | |||
| package otel | |||
There was a problem hiding this comment.
Should this go to its own package the same way as KeyValue?
number.NewInt64() is better than otel.NewInt64Number()
| type HandleImpl interface { | ||
| // Handle interface for implementations of individual metrics with precomputed | ||
| // labels. | ||
| type Handle interface { |
There was a problem hiding this comment.
This is confusing, what does otel.Handle means? Maybe change to MetricsHandle?
| type InstrumentImpl interface { | ||
| // Instrument interface for implementations of individual metrics without | ||
| // precomputed labels. | ||
| type Instrument interface { |
There was a problem hiding this comment.
This is confusing, what does otel.Instrument means? Maybe change to MetricsInstrument?
| return WithMap(ctx, FromContext(ctx).Apply(MapUpdate{ | ||
| MultiKV: keyvalues, | ||
| })) | ||
| } | ||
|
|
||
| // FromContext extracts a Map from the Context if it exists, otherwise returns | ||
| // an empty Map. | ||
| func FromContext(ctx context.Context) Map { |
There was a problem hiding this comment.
I think otel.FromContext doesn't capture well what this does. What are we trying to get from context?
|
I'm going to close this an open a new PR that focuses on specific parts of the api. |
The goal is to explore consolidating the various
go.opentelemetry.io/otel/api/*packages into a coherent basego.opentelemetry.io/otelpackage.IMO with this (plus additional follow on cleanups), the otel package is more coherent than the multiple api packages were. I don't think it's perfect though, but IMO that is because I've only done the slightest bit of consolidation, renaming, documentation updates.