Skip to content

Commit

Permalink
#142 follow spec for OTLP protocol (#148)
Browse files Browse the repository at this point in the history 
Per #142, recent PRs had otel-cli deviate from the OTel standards for selecting OTLP exporter protocol. This PR revises the protocol handling for `--endpoint` and adds `--protocol` to more closely or exactly follow the spec. Matching envvars `OTEL_EXPORTER_OTLP_PROTOCOL` and `OTEL_EXPORTER_OTLP_TRACES_PROTOCOL` have been added as well.

From the README.md:

otel-cli deviates from the OTel specification for endpoint URIs. Mainly, otel-cli supports bare host:port for grpc endpoints and continues to default to gRPC. The optional http/json is not supported by opentelemetry-go so otel-cli does not support it. To use gRPC with an http endpoint, set the protocol with --protocol or the envvar.

    * bare host:port endpoints are assumed to be gRPC and are not supported for HTTP
    * http:// and https:// are assumed to be HTTP unless --protocol is set to grpc.
    * loopback addresses without an https:// prefix are assumed to be unencrypted

* go mod tidy

seems to be cruft leftover from the renovate bot merge

* add --protocol flag & envvar + failing tests

Adds the flag & envvars along with a bunch of tests that mostly fail at
this point. Once these all pass it should be good to ship.

* set correct test server protocol on new tests

* minor improvements to failure testing

Don't try to parse JSON output when command is expected to fail.

Initialize empty maps in results so they don't come back as nil
in failure tests.

* tune up tests, get them to pass

Lots of little adjustments all over.

gRPC with http endpoints now works when protocol is grpc.

Protocol flag & envvars seem to be fully implemented.

Removed mentions of http/json since opentelemetry-go doesn't support it.

Updated README to reflect new functionality.

* fix up README text

* remove OtelError from tests

It doesn't work anyways, will revisit when/if it's needed.
  • Loading branch information
@tobert
tobert authored Jan 31, 2023
1 parent 72df644 commit e8b86f6
Show file tree
Hide file tree
Showing 8 changed files with 180 additions and 32 deletions.
11 changes: 7 additions & 4 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ then config file, then environment variables.
| CLI argument | environment variable | config file key | example value |
| -------------------- | ----------------------------- | ------------------------ | -------------- |
| --endpoint | OTEL_EXPORTER_OTLP_ENDPOINT | endpoint | localhost:4317 |
| --protocol | OTEL_EXPORTER_OTLP_PROTOCOL | protocol | http/protobuf |
| --insecure | OTEL_EXPORTER_OTLP_INSECURE | insecure | false |
| --timeout | OTEL_EXPORTER_OTLP_TIMEOUT | timeout | 1s |
| --otlp-headers | OTEL_EXPORTER_OTLP_HEADERS | otlp_headers | k=v,a=b |
Expand All @@ -126,11 +127,13 @@ then config file, then environment variables.

### Endpoint URIs

otel-cli deviates from the OTel specification for endpoint URIs.
otel-cli deviates from the OTel specification for endpoint URIs. Mainly, otel-cli supports
bare host:port for grpc endpoints and continues to default to gRPC. The optional http/json
is not supported by opentelemetry-go so otel-cli does not support it. To use gRPC with an
http endpoint, set the protocol with --protocol or the envvar.

* bare `host:port` endpoints are assumed to be gRPC
* `grpc://` URIs are supported
* `http://` and `https://` are assumed to be HTTP _only_ (the spec reuses them for gRPC)
* bare `host:port` endpoints are assumed to be gRPC and are not supported for HTTP
* `http://` and `https://` are assumed to be HTTP unless --protocol is set to `grpc`.
* loopback addresses without an https:// prefix are assumed to be unencrypted

## Easy local dev
Expand Down
144 changes: 140 additions & 4 deletions data_for_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ var suites = []FixtureSuite{
Diagnostics: otelcli.Diagnostics{
IsRecording: false,
NumArgs: 1,
OtelError: "",
},
},
},
Expand Down Expand Up @@ -107,7 +106,6 @@ var suites = []FixtureSuite{
NumArgs: 3,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
OtelError: "",
},
Spans: 1,
},
Expand All @@ -126,14 +124,13 @@ var suites = []FixtureSuite{
SpanData: map[string]string{
"span_id": "*",
"trace_id": "*",
"server_meta": "host={{endpoint}},method=POST,proto=HTTP/1.1,uri=/v1/traces",
"server_meta": "content-type=application/x-protobuf,host={{endpoint}},method=POST,proto=HTTP/1.1,uri=/v1/traces",
},
Diagnostics: otelcli.Diagnostics{
IsRecording: true,
NumArgs: 3,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
OtelError: "",
},
Spans: 1,
},
Expand Down Expand Up @@ -445,4 +442,143 @@ var suites = []FixtureSuite{
},
},
},
// validate OTEL_EXPORTER_OTLP_PROTOCOL / --protocol
{
// --protocol
{
Name: "--protocol grpc",
Config: FixtureConfig{
ServerProtocol: grpcProtocol,
CliArgs: []string{"status", "--endpoint", "{{endpoint}}", "--protocol", "grpc"},
TestTimeoutMs: 1000,
},
Expect: Results{
Config: otelcli.DefaultConfig().WithEndpoint("{{endpoint}}").WithProtocol("grpc"),
SpanData: map[string]string{
"server_meta": "proto=grpc",
},
Diagnostics: otelcli.Diagnostics{
IsRecording: true,
NumArgs: 5,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
},
Spans: 1,
},
},
{
Name: "--protocol http/protobuf",
Config: FixtureConfig{
ServerProtocol: httpProtocol,
CliArgs: []string{"status", "--endpoint", "http://{{endpoint}}", "--protocol", "http/protobuf"},
TestTimeoutMs: 1000,
},
Expect: Results{
Config: otelcli.DefaultConfig().WithEndpoint("http://{{endpoint}}").WithProtocol("http/protobuf"),
SpanData: map[string]string{
"server_meta": "content-type=application/x-protobuf,host={{endpoint}},method=POST,proto=HTTP/1.1,uri=/v1/traces",
},
Diagnostics: otelcli.Diagnostics{
IsRecording: true,
NumArgs: 5,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
},
Spans: 1,
},
},
{
Name: "protocol: bad config",
Config: FixtureConfig{
CliArgs: []string{"status", "--endpoint", "{{endpoint}}", "--protocol", "xxx", "--verbose", "--fail"},
TestTimeoutMs: 1000,
},
Expect: Results{
CommandFailed: true,
CliOutputRe: regexp.MustCompile(`^\d{4}/\d{2}/\d{2} \d{2}:\d{2}:\d{2} `),
CliOutput: "invalid protocol setting \"xxx\"\n",
Config: otelcli.DefaultConfig().WithEndpoint("{{endpoint}}"),
Diagnostics: otelcli.Diagnostics{
IsRecording: false,
NumArgs: 7,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
},
Spans: 0,
},
},
// OTEL_EXPORTER_OTLP_PROTOCOL
{
Name: "OTEL_EXPORTER_OTLP_PROTOCOL grpc",
Config: FixtureConfig{
ServerProtocol: grpcProtocol,
// validate protocol can be set to grpc with an http endpoint
CliArgs: []string{"status", "--endpoint", "http://{{endpoint}}"},
TestTimeoutMs: 1000,
Env: map[string]string{
"OTEL_EXPORTER_OTLP_PROTOCOL": "grpc",
},
},
Expect: Results{
Config: otelcli.DefaultConfig().WithEndpoint("http://{{endpoint}}").WithProtocol("grpc"),
SpanData: map[string]string{
"server_meta": "proto=grpc",
},
Diagnostics: otelcli.Diagnostics{
IsRecording: true,
NumArgs: 3,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
},
Spans: 1,
},
},
{
Name: "OTEL_EXPORTER_OTLP_PROTOCOL http/protobuf",
Config: FixtureConfig{
ServerProtocol: httpProtocol,
CliArgs: []string{"status", "--endpoint", "http://{{endpoint}}"},
TestTimeoutMs: 1000,
Env: map[string]string{
"OTEL_EXPORTER_OTLP_PROTOCOL": "http/protobuf",
},
},
Expect: Results{
Config: otelcli.DefaultConfig().WithEndpoint("http://{{endpoint}}").WithProtocol("http/protobuf"),
SpanData: map[string]string{
"server_meta": "content-type=application/x-protobuf,host={{endpoint}},method=POST,proto=HTTP/1.1,uri=/v1/traces",
},
Diagnostics: otelcli.Diagnostics{
IsRecording: true,
NumArgs: 3,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
},
Spans: 1,
},
},
{
Name: "OTEL_EXPORTER_OTLP_PROTOCOL: bad config",
Config: FixtureConfig{
CliArgs: []string{"status", "--endpoint", "http://{{endpoint}}", "--fail", "--verbose"},
TestTimeoutMs: 1000,
Env: map[string]string{
"OTEL_EXPORTER_OTLP_PROTOCOL": "roflcopter",
},
},
Expect: Results{
CommandFailed: true,
CliOutputRe: regexp.MustCompile(`^\d{4}/\d{2}/\d{2} \d{2}:\d{2}:\d{2} `),
CliOutput: "invalid protocol setting \"roflcopter\"\n",
Config: otelcli.DefaultConfig().WithEndpoint("http://{{endpoint}}"),
Diagnostics: otelcli.Diagnostics{
IsRecording: false,
NumArgs: 3,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
},
Spans: 0,
},
},
},
}
14 changes: 0 additions & 14 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,32 +215,18 @@ go.opencensus.io v0.22.0/go.mod h1:+kGneAE2xo2IficOXnaByMWTGM9T73dGwxeWcUqIpI8=
go.opencensus.io v0.22.2/go.mod h1:yxeiOL68Rb0Xd1ddK5vPZ/oVn4vY4Ynel7k9FzqtOIw=
go.opencensus.io v0.22.3/go.mod h1:yxeiOL68Rb0Xd1ddK5vPZ/oVn4vY4Ynel7k9FzqtOIw=
go.opencensus.io v0.22.4/go.mod h1:yxeiOL68Rb0Xd1ddK5vPZ/oVn4vY4Ynel7k9FzqtOIw=
go.opentelemetry.io/otel v1.11.2 h1:YBZcQlsVekzFsFbjygXMOXSs6pialIZxcjfO/mBDmR0=
go.opentelemetry.io/otel v1.11.2/go.mod h1:7p4EUV+AqgdlNV9gL97IgUZiVR3yrFXYo53f9BM3tRI=
go.opentelemetry.io/otel v1.12.0 h1:IgfC7kqQrRccIKuB7Cl+SRUmsKbEwSGPr0Eu+/ht1SQ=
go.opentelemetry.io/otel v1.12.0/go.mod h1:geaoz0L0r1BEOR81k7/n9W4TCXYCJ7bPO7K374jQHG0=
go.opentelemetry.io/otel/exporters/otlp/internal/retry v1.11.2 h1:htgM8vZIF8oPSCxa341e3IZ4yr/sKxgu8KZYllByiVY=
go.opentelemetry.io/otel/exporters/otlp/internal/retry v1.11.2/go.mod h1:rqbht/LlhVBgn5+k3M5QK96K5Xb0DvXpMJ5SFQpY6uw=
go.opentelemetry.io/otel/exporters/otlp/internal/retry v1.12.0 h1:UfDENi+LTcLjQ/JhaXimjlIgn7wWjwbEMmdREm2Gyng=
go.opentelemetry.io/otel/exporters/otlp/internal/retry v1.12.0/go.mod h1:rqbht/LlhVBgn5+k3M5QK96K5Xb0DvXpMJ5SFQpY6uw=
go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.11.2 h1:fqR1kli93643au1RKo0Uma3d2aPQKT+WBKfTSBaKbOc=
go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.11.2/go.mod h1:5Qn6qvgkMsLDX+sYK64rHb1FPhpn0UtxF+ouX1uhyJE=
go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.12.0 h1:ZVqtSAxrR4+ofzayuww0/EKamCjjnwnXTMRZzMudJoU=
go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.12.0/go.mod h1:IlaGLENJkAl9+Xoo3J0unkdOwtL+rmqZ3ryMjUtYA94=
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.11.2 h1:ERwKPn9Aer7Gxsc0+ZlutlH1bEEAUXAUhqm3Y45ABbk=
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.11.2/go.mod h1:jWZUM2MWhWCJ9J9xVbRx7tzK1mXKpAlze4CeulycwVY=
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.12.0 h1:+tsVdWosoqDfX6cdHAeacZozjQS94ySBd+aUXFwnNKA=
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.12.0/go.mod h1:jSqjV+Knu1Jyvh+l3fx7V210Ev3HHgNQAi8YqpXaQP8=
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.11.2 h1:Us8tbCmuN16zAnK5TC69AtODLycKbwnskQzaB6DfFhc=
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.11.2/go.mod h1:GZWSQQky8AgdJj50r1KJm8oiQiIPaAX7uZCFQX9GzC8=
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.12.0 h1:L23MzcHDznr05xOM1Ng1F98L0nVd7hm/S7y2jW9IRB4=
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.12.0/go.mod h1:C+onYX2j5QH653b3wGJwowYr8jLMjBJw35QcaCQQK0U=
go.opentelemetry.io/otel/sdk v1.11.2 h1:GF4JoaEx7iihdMFu30sOyRx52HDHOkl9xQ8SMqNXUiU=
go.opentelemetry.io/otel/sdk v1.11.2/go.mod h1:wZ1WxImwpq+lVRo4vsmSOxdd+xwoUJ6rqyLc3SyX9aU=
go.opentelemetry.io/otel/sdk v1.12.0 h1:8npliVYV7qc0t1FKdpU08eMnOjgPFMnriPhn0HH4q3o=
go.opentelemetry.io/otel/sdk v1.12.0/go.mod h1:WYcvtgquYvgODEvxOry5owO2y9MyciW7JqMz6cpXShE=
go.opentelemetry.io/otel/trace v1.11.2 h1:Xf7hWSF2Glv0DE3MH7fBHvtpSBsjcBUe5MYAmZM/+y0=
go.opentelemetry.io/otel/trace v1.11.2/go.mod h1:4N+yC7QEz7TTsG9BSRLNAa63eg5E06ObSbKPmxQ/pKA=
go.opentelemetry.io/otel/trace v1.12.0 h1:p28in++7Kd0r2d8gSt931O57fdjUyWxkVbESuILAeUc=
go.opentelemetry.io/otel/trace v1.12.0/go.mod h1:pHlgBynn6s25qJ2szD+Bv+iwKJttjHSI3lUAyf0GNuQ=
go.opentelemetry.io/proto/otlp v0.7.0/go.mod h1:PqfVotwruBrMGOCsRd/89rSnXhoiJIqeYNgFYFoEGnI=
Expand Down
9 changes: 6 additions & 3 deletions main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ func checkAll(t *testing.T, fixture Fixture, endpoint string, results Results, s

// many of the basic plumbing tests use status so it has its own set of checks
// but these shouldn't run for testing the other subcommands
if len(fixture.Config.CliArgs) > 0 && fixture.Config.CliArgs[0] == "status" {
if len(fixture.Config.CliArgs) > 0 && fixture.Config.CliArgs[0] == "status" && !fixture.Expect.CommandFailed {
checkStatusData(t, fixture, endpoint, results)
} else {
// checking the text output only makes sense for non-status paths
Expand Down Expand Up @@ -296,7 +296,10 @@ func checkSpanData(t *testing.T, fixture Fixture, endpoint string, span otlpserv
func runOtelCli(t *testing.T, fixture Fixture) (string, Results, otlpserver.CliEvent, otlpserver.CliEventList) {
started := time.Now()

var results Results
results := Results{
SpanData: map[string]string{},
Env: map[string]string{},
}
var retSpan otlpserver.CliEvent
var retEvents otlpserver.CliEventList

Expand Down Expand Up @@ -409,7 +412,7 @@ func runOtelCli(t *testing.T, fixture Fixture) (string, Results, otlpserver.CliE

// only try to parse status json if it was a status command
// TODO: support variations on otel-cli where status isn't the first arg
if len(cliOut) > 0 && len(args) > 0 && args[0] == "status" {
if len(cliOut) > 0 && len(args) > 0 && args[0] == "status" && !fixture.Expect.CommandFailed {
err = json.Unmarshal(cliOut, &results)
if err != nil {
t.Errorf("[%s] parsing otel-cli status output failed: %s", fixture.Name, err)
Expand Down
9 changes: 9 additions & 0 deletions otelcli/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ const spanBgSockfilename = "otel-cli-background.sock"
func DefaultConfig() Config {
return Config{
Endpoint: "",
Protocol: "",
Timeout: "1s",
Headers: map[string]string{},
Insecure: false,
Expand Down Expand Up @@ -58,6 +59,7 @@ func DefaultConfig() Config {
// Data structure is public so that it can serialize to json easily.
type Config struct {
Endpoint string `json:"endpoint" env:"OTEL_EXPORTER_OTLP_ENDPOINT,OTEL_EXPORTER_OTLP_TRACES_ENDPOINT"`
Protocol string `json:"protocol" env:"OTEL_EXPORTER_OTLP_PROTOCOL,OTEL_EXPORTER_OTLP_TRACES_PROTOCOL"`
Timeout string `json:"timeout" env:"OTEL_EXPORTER_OTLP_TIMEOUT,OTEL_EXPORTER_OTLP_TRACES_TIMEOUT"`
Headers map[string]string `json:"otlp_headers" env:"OTEL_EXPORTER_OTLP_HEADERS"` // TODO: needs json marshaler hook to mask tokens
Insecure bool `json:"insecure" env:"OTEL_EXPORTER_OTLP_INSECURE"`
Expand Down Expand Up @@ -171,6 +173,7 @@ func (c *Config) LoadEnv(getenv func(string) string) error {
func (c Config) ToStringMap() map[string]string {
return map[string]string{
"endpoint": c.Endpoint,
"protocol": c.Protocol,
"timeout": c.Timeout,
"headers": flattenStringMap(c.Headers, "{}"),
"insecure": strconv.FormatBool(c.Insecure),
Expand Down Expand Up @@ -205,6 +208,12 @@ func (c Config) WithEndpoint(with string) Config {
return c
}

// WithProtocol returns the config with protocol set to the provided value.
func (c Config) WithProtocol(with string) Config {
c.Protocol = with
return c
}

// WithTimeout returns the config with Timeout set to the provided value.
func (c Config) WithTimeout(with string) Config {
c.Timeout = with
Expand Down
14 changes: 11 additions & 3 deletions otelcli/plumbing.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ func initTracer() (context.Context, func()) {
return ctx, func() {}
}

if config.Protocol != "" && config.Protocol != "grpc" && config.Protocol != "http/protobuf" {
softFail("invalid protocol setting %q", config.Protocol)
}

var exporter sdktrace.SpanExporter // allows overwrite in --test mode
var err error

Expand All @@ -41,8 +45,10 @@ func initTracer() (context.Context, func()) {
// awkward for otel-cli so we break with the spec. otel-cli will only resolve
// http(s):// to HTTP protocols, defaults bare host:port to gRPC, and supports
// grpc:// to definitely use gRPC to connect out.
if strings.HasPrefix(config.Endpoint, "http://") ||
strings.HasPrefix(config.Endpoint, "https://") {
if config.Protocol != "grpc" &&
(strings.HasPrefix(config.Protocol, "http/") ||
strings.HasPrefix(config.Endpoint, "http://") ||
strings.HasPrefix(config.Endpoint, "https://")) {
exporter, err = otlphttp.New(ctx, httpOptions()...)
if err != nil {
softFail("failed to configure OTLP/HTTP exporter: %s", err)
Expand Down Expand Up @@ -97,7 +103,9 @@ func grpcOptions() []otlpgrpc.Option {
grpcOpts := []otlpgrpc.Option{}

// per comment in initTracer(), grpc:// is specific to otel-cli
if strings.HasPrefix(config.Endpoint, "grpc://") {
if strings.HasPrefix(config.Endpoint, "grpc://") ||
strings.HasPrefix(config.Endpoint, "http://") ||
strings.HasPrefix(config.Endpoint, "https://") {
ep, err := url.Parse(config.Endpoint)
if err != nil {
softFail("error parsing provided gRPC URI '%s': %s", config.Endpoint, err)
Expand Down
2 changes: 2 additions & 0 deletions otelcli/root.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ func addCommonParams(cmd *cobra.Command) {
cmd.Flags().StringVarP(&config.CfgFile, "config", "c", defaults.CfgFile, "JSON configuration file")
// --endpoint an endpoint to send otlp output to
cmd.Flags().StringVar(&config.Endpoint, "endpoint", defaults.Endpoint, "host and port for the desired OTLP/gRPC or OTLP/HTTP endpoint (use http:// or https:// for OTLP/HTTP)")
// --protocol allows setting the OTLP protocol instead of relying on auto-detection from URI
cmd.Flags().StringVar(&config.Protocol, "protocol", defaults.Protocol, "desired OTLP protocol: grpc or http/protobuf")
// --timeout a default timeout to use in all otel-cli operations (default 1s)
cmd.Flags().StringVar(&config.Timeout, "timeout", defaults.Timeout, "timeout for otel-cli operations, all timeouts in otel-cli use this value")
// --verbose tells otel-cli to actually log errors to stderr instead of failing silently
Expand Down
9 changes: 5 additions & 4 deletions otlpserver/httpserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,11 @@ func (hs *HttpServer) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
}

meta := map[string]string{
"method": req.Method,
"proto": req.Proto,
"host": req.Host,
"uri": req.RequestURI,
"method": req.Method,
"proto": req.Proto,
"content-type": req.Header.Get("Content-Type"),
"host": req.Host,
"uri": req.RequestURI,
}

done := otelToCliEvent(hs.callback, &msg, meta)
Expand Down

0 comments on commit e8b86f6

Please sign in to comment.