Skip to content

Commit

Permalink
fix endpoint paths and signal-specific behavior (#200) (#214)
Browse files Browse the repository at this point in the history 
* add TracesEndpoint field to Config

* add Endpoint and EndpointSource fields to diagnostics

* add * matching to Diagnostics checks, shorten output on failures

Modified checkProcess to return a bool if the process met the expected
conditions. If it failed unexpectedly, checkAll will bail out of running
further checks on the data, so output is a lot less spammy.

Added "*" support to the check on diagnostics data strings.

* add diagnostics fields, add tests for #200

Adds 2 tests to the regression suite to validate endpoints on general
get /v1/traces appended, and that signal endpoints do not.

* add --traces-endpoint option

* refactor endpoint parsing to match OTel specs

Moved URL parsing to its own func, moving some code from the http/grpc
option paths to it, as well as starting to consider the "source" of
general (e.g. --endpoint, OTEL_EXPORTER_OTLP_ENDPOINT) or a signal
(e.g. --signal-endpoint or OTEL_EXPORTER_OTLP_TRACES_ENDPOINT). They are
treated differently now to match the spec.

Added tests for parseEndpoint.

* automatically set port for bare hostname + gRPC

e.g. "localhost", "127.0.0.1" are always going to go to gRPC, and when
no port is provided, should default to 4317.

Might need to check the spec. It probably doesn't allow for bare
hostname but otel-cli will because it should be easy to type.

* update tests to account for default port on bare hostnames
  • Loading branch information
@tobert
tobert authored May 26, 2023
1 parent 8f9f2f5 commit 2060787
Show file tree
Hide file tree
Showing 8 changed files with 263 additions and 65 deletions.
71 changes: 71 additions & 0 deletions data_for_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ var suites = []FixtureSuite{
NumArgs: 3,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
Endpoint: "*",
EndpointSource: "*",
},
SpanCount: 1,
},
Expand All @@ -148,6 +150,8 @@ var suites = []FixtureSuite{
NumArgs: 3,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
Endpoint: "*",
EndpointSource: "*",
},
SpanCount: 1,
},
Expand Down Expand Up @@ -182,6 +186,8 @@ var suites = []FixtureSuite{
DetectedLocalhost: true,
InsecureSkipVerify: true,
ParsedTimeoutMs: 1000,
Endpoint: "*",
EndpointSource: "*",
},
SpanCount: 1,
},
Expand All @@ -204,6 +210,8 @@ var suites = []FixtureSuite{
NumArgs: 4,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
Endpoint: "*",
EndpointSource: "*",
},
SpanCount: 1,
},
Expand Down Expand Up @@ -239,6 +247,8 @@ var suites = []FixtureSuite{
DetectedLocalhost: true,
InsecureSkipVerify: true,
ParsedTimeoutMs: 1000,
Endpoint: "*",
EndpointSource: "*",
},
SpanCount: 1,
},
Expand Down Expand Up @@ -271,6 +281,8 @@ var suites = []FixtureSuite{
NumArgs: 11,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
Endpoint: "*",
EndpointSource: "*",
},
SpanCount: 1,
},
Expand Down Expand Up @@ -331,6 +343,8 @@ var suites = []FixtureSuite{
NumArgs: 3,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
Endpoint: "*",
EndpointSource: "*",
OtelError: `Post "https://{{endpoint}}/v1/traces": http: server gave HTTP response to HTTPS client`,
},
SpanCount: 0,
Expand Down Expand Up @@ -388,6 +402,47 @@ var suites = []FixtureSuite{
DetectedLocalhost: true,
NumArgs: 1,
ParsedTimeoutMs: 1000,
Endpoint: "*",
EndpointSource: "*",
},
},
},
{
Name: "#200 custom trace path in general endpoint gets signal path appended",
Config: FixtureConfig{
CliArgs: []string{"status", "--endpoint", "http://{{endpoint}}/mycollector"},
ServerProtocol: httpProtocol,
},
Expect: Results{
SpanCount: 1,
Config: otelcli.DefaultConfig().WithEndpoint("http://{{endpoint}}/mycollector"),
Diagnostics: otelcli.Diagnostics{
IsRecording: true,
DetectedLocalhost: true,
NumArgs: 3,
ParsedTimeoutMs: 1000,
// spec says /v1/traces should get appended to any general endpoint URL
Endpoint: "http://{{endpoint}}/mycollector/v1/traces",
EndpointSource: "general",
},
},
},
{
Name: "#200 custom trace path on signal endpoint does not get modified",
Config: FixtureConfig{
CliArgs: []string{"status", "--traces-endpoint", "http://{{endpoint}}/mycollector/x/1"},
ServerProtocol: httpProtocol,
},
Expect: Results{
SpanCount: 1,
Config: otelcli.DefaultConfig().WithTracesEndpoint("http://{{endpoint}}/mycollector/x/1"),
Diagnostics: otelcli.Diagnostics{
IsRecording: true,
DetectedLocalhost: true,
NumArgs: 3,
ParsedTimeoutMs: 1000,
Endpoint: "http://{{endpoint}}/mycollector/x/1",
EndpointSource: "signal",
},
},
},
Expand Down Expand Up @@ -420,6 +475,8 @@ var suites = []FixtureSuite{
IsRecording: true,
NumArgs: 3,
ParsedTimeoutMs: 1000,
Endpoint: "*",
EndpointSource: "*",
},
Env: map[string]string{
"OTEL_EXPORTER_OTLP_ENDPOINT": "{{endpoint}}",
Expand Down Expand Up @@ -720,6 +777,8 @@ var suites = []FixtureSuite{
NumArgs: 5,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
Endpoint: "*",
EndpointSource: "*",
},
SpanCount: 1,
},
Expand All @@ -741,6 +800,8 @@ var suites = []FixtureSuite{
NumArgs: 5,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
Endpoint: "*",
EndpointSource: "*",
},
SpanCount: 1,
},
Expand All @@ -761,6 +822,8 @@ var suites = []FixtureSuite{
NumArgs: 7,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
Endpoint: "*",
EndpointSource: "*",
},
SpanCount: 0,
},
Expand Down Expand Up @@ -790,6 +853,8 @@ var suites = []FixtureSuite{
NumArgs: 3,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
Endpoint: "*",
EndpointSource: "*",
},
SpanCount: 1,
},
Expand Down Expand Up @@ -817,6 +882,8 @@ var suites = []FixtureSuite{
NumArgs: 3,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
Endpoint: "*",
EndpointSource: "*",
},
SpanCount: 1,
},
Expand All @@ -840,6 +907,8 @@ var suites = []FixtureSuite{
NumArgs: 3,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
Endpoint: "*",
EndpointSource: "*",
OtelError: "invalid protocol setting \"roflcopter\"\n",
},
SpanCount: 0,
Expand Down Expand Up @@ -870,6 +939,8 @@ var suites = []FixtureSuite{
IsRecording: true,
DetectedLocalhost: true,
ParsedTimeoutMs: 1000,
Endpoint: "*",
EndpointSource: "*",
},
},
},
Expand Down
17 changes: 15 additions & 2 deletions main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,12 @@ func runFixture(t *testing.T, fixture Fixture, wait, done chan struct{}) {
// checkAll gathers up all the check* funcs below into one function.
func checkAll(t *testing.T, fixture Fixture, results Results) {
// check timeout and process status expectations
checkProcess(t, fixture, results)
success := checkProcess(t, fixture, results)
// when the process fails, no point in checking the rest, it's just noise
if !success {
t.Log("otel-cli process failed unexpectedly, will not test values from it")
return
}

// compare the number of recorded spans against expectations in the fixture
checkSpanCount(t, fixture, results)
Expand Down Expand Up @@ -179,13 +184,16 @@ func checkSpanCount(t *testing.T, fixture Fixture, results Results) {
// checkProcess validates configured expectations about whether otel-cli failed
// or the test timed out. These are mostly used for testing that otel-cli fails
// in the way we want it to.
func checkProcess(t *testing.T, fixture Fixture, results Results) {
func checkProcess(t *testing.T, fixture Fixture, results Results) bool {
if results.TimedOut != fixture.Expect.TimedOut {
t.Errorf("[%s] test timeout status is %t but expected %t", fixture.Name, results.TimedOut, fixture.Expect.TimedOut)
return false
}
if results.CommandFailed != fixture.Expect.CommandFailed {
t.Errorf("[%s] command failed is %t but expected %t", fixture.Name, results.CommandFailed, fixture.Expect.CommandFailed)
return false
}
return true
}

// checkOutput looks that otel-cli output stored in the results and compares against
Expand Down Expand Up @@ -222,6 +230,11 @@ func checkStatusData(t *testing.T, fixture Fixture, results Results) {
if wantDiag["cli_args"] == "" {
gotDiag["cli_args"] = ""
}
for k, v := range wantDiag {
if v == "*" {
wantDiag[k] = gotDiag[k]
}
}
if diff := cmp.Diff(wantDiag, gotDiag); diff != "" {
t.Errorf("[%s] diagnostic data did not match fixture (-want +got):\n%s", fixture.Name, diff)
}
Expand Down
21 changes: 14 additions & 7 deletions otelcli/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,13 @@ func DefaultConfig() Config {
// This is used as a singleton as "config" and accessed from many other files.
// 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"`
Blocking bool `json:"otlp_blocking" env:"OTEL_EXPORTER_OTLP_BLOCKING"`
Endpoint string `json:"endpoint" env:"OTEL_EXPORTER_OTLP_ENDPOINT"`
TracesEndpoint string `json:"traces_endpoint" env:"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"`
Blocking bool `json:"otlp_blocking" env:"OTEL_EXPORTER_OTLP_BLOCKING"`

TlsCACert string `json:"tls_ca_cert" env:"OTEL_EXPORTER_OTLP_CERTIFICATE,OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE"`
TlsClientKey string `json:"tls_client_key" env:"OTEL_EXPORTER_OTLP_CLIENT_KEY,OTEL_EXPORTER_OTLP_TRACES_CLIENT_KEY"`
Expand Down Expand Up @@ -226,7 +227,7 @@ func (c Config) ToStringMap() map[string]string {
// IsRecording returns true if an endpoint is set and otel-cli expects to send real
// spans. Returns false if unconfigured and going to run inert.
func (c Config) IsRecording() bool {
if c.Endpoint == "" {
if c.Endpoint == "" && c.TracesEndpoint == "" {
diagnostics.IsRecording = false
return false
}
Expand All @@ -241,6 +242,12 @@ func (c Config) WithEndpoint(with string) Config {
return c
}

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

// WithProtocol returns the config with protocol set to the provided value.
func (c Config) WithProtocol(with string) Config {
c.Protocol = with
Expand Down
5 changes: 5 additions & 0 deletions otelcli/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ func TestWithEndpoint(t *testing.T) {
t.Fail()
}
}
func TestWithTracesEndpoint(t *testing.T) {
if DefaultConfig().WithTracesEndpoint("foobar").TracesEndpoint != "foobar" {
t.Fail()
}
}
func TestWithTimeout(t *testing.T) {
if DefaultConfig().WithTimeout("foobar").Timeout != "foobar" {
t.Fail()
Expand Down
4 changes: 4 additions & 0 deletions otelcli/diagnostics.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ type Diagnostics struct {
DetectedLocalhost bool `json:"detected_localhost"`
InsecureSkipVerify bool `json:"insecure_skip_verify"`
ParsedTimeoutMs int64 `json:"parsed_timeout_ms"`
Endpoint string `json:"endpoint"` // the computed endpoint, not the raw config val
EndpointSource string `json:"endpoint_source"`
OtelError string `json:"otel_error"`
ExecExitCode int `json:"exec_exit_code"`
config Config
Expand Down Expand Up @@ -47,6 +49,8 @@ func (d *Diagnostics) ToStringMap() map[string]string {
"number_of_args": strconv.Itoa(d.NumArgs),
"detected_localhost": strconv.FormatBool(d.DetectedLocalhost),
"parsed_timeout_ms": strconv.FormatInt(d.ParsedTimeoutMs, 10),
"endpoint": d.Endpoint,
"endpoint_source": d.EndpointSource,
"otel_error": d.OtelError,
}
}
Expand Down
Loading

0 comments on commit 2060787

Please sign in to comment.