Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix otlp exporters default endpoints #4155

Closed
wants to merge 5 commits into from
Closed

Fix otlp exporters default endpoints #4155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a4a81a2
Fix default endpoints for otlptrace package
remychantenay May 31, 2023
8045992
Fix default endpoints for otlpmetric package
remychantenay May 31, 2023
9f10b14
Update CHANGELOG.md
remychantenay May 31, 2023
ab5d4ff
Update CHANGELOG.md
pellared Jun 1, 2023
97b04ac
Merge branch 'main' into fix-otlp-exporters-default-endpoints
hanyuancheung Jun 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

## [Unreleased]

### Fixed

- Fix default endpoint (use `http` instead of `https`) for OTLP exporters. (#4155)

## [1.16.0/0.39.0] 2023-05-18

This release contains the first stable release of the OpenTelemetry Go [metric API].
Expand Down
23 changes: 20 additions & 3 deletions exporters/otlp/otlpmetric/internal/oconf/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,16 +89,23 @@ func NewHTTPConfig(opts ...HTTPOption) Config {
URLPath: DefaultMetricsPath,
Compression: NoCompression,
Timeout: DefaultTimeout,
Insecure: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default for this should be false according to the specification: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.21.0/specification/protocol/exporter.md#configuration-options

Why is this set to true here?

Copy link
Member

@pellared pellared May 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just want to add a not that the specification is not worded perfectly.

This option only applies to OTLP/gRPC when an endpoint is provided without the http or https scheme.

insecure is by default true when an endpoint is provided without the http or https scheme.

Still, I think that we should keep the false as default and not interpret the Insecure field if the HTTP scheme is defined. (I guess this is what @MrAlias has in mind)

Copy link
Contributor Author

@remychantenay remychantenay Jun 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you both. Looks like there is a bit of confusion from my end. I will re-read the specs with rested eyes over the weekend.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@remychantenay

The wording for insecure says

This option only applies to OTLP/gRPC when an endpoint is provided without the http or https scheme - OTLP/HTTP always uses the scheme provided for the endpoint.

My interpretation of this is that OTLP/HTTP endpoints must include a scheme (as noted n the specification). But the scheme overrides whatever setting there may be for Insecure in the config (i.e. it's a noop setting for OTLP/HTTP).

For example, if the OTLP/HTTP endpoint starts with http, then the exporter should act as though Insecure= true, but if the scheme in the HTTP endpoint is https, it should act as ifInsecure=false. The wording seems to indicate that the user setting for Insecure shouldn't be changed, but rather the exporter should just use the appropriate setting regardless of the Insecure value in the config.

For OTLP/gRPC, there are more options for specifying the endpoint. But if the gRPC endpoint includes an http or https scheme, then the same "ignore what the setting says and act as though it is whatever the scheme says" applies the same as in OTLP/HTTP.

But if the gRPC endpoint does not include an http or https scheme, then the Insecure setting should be taken into account and should default to Insecure=false.


TemporalitySelector: metric.DefaultTemporalitySelector,
AggregationSelector: metric.DefaultAggregationSelector,
},
RetryConfig: retry.DefaultConfig,
}

cfg = ApplyHTTPEnvConfigs(cfg)
for _, opt := range opts {
cfg = opt.ApplyHTTPOption(cfg)
}

if cfg.Metrics.TLSCfg != nil {
cfg.Metrics.Insecure = false
}

cfg.Metrics.URLPath = internal.CleanPath(cfg.Metrics.URLPath, DefaultMetricsPath)
return cfg
}
Expand All @@ -112,26 +119,36 @@ func NewGRPCConfig(opts ...GRPCOption) Config {
URLPath: DefaultMetricsPath,
Compression: NoCompression,
Timeout: DefaultTimeout,
Insecure: true,

TemporalitySelector: metric.DefaultTemporalitySelector,
AggregationSelector: metric.DefaultAggregationSelector,
},
RetryConfig: retry.DefaultConfig,
DialOptions: []grpc.DialOption{grpc.WithUserAgent(ominternal.GetUserAgentHeader())},
}

cfg = ApplyGRPCEnvConfigs(cfg)
for _, opt := range opts {
cfg = opt.ApplyGRPCOption(cfg)
}

if cfg.Metrics.TLSCfg != nil {
cfg.Metrics.Insecure = false
}

if cfg.ServiceConfig != "" {
cfg.DialOptions = append(cfg.DialOptions, grpc.WithDefaultServiceConfig(cfg.ServiceConfig))
}
// Priroritize GRPCCredentials over Insecure (passing both is an error).
// Prioritize GRPCCredentials over Insecure (passing both is an error).
if cfg.Metrics.GRPCCredentials != nil {
cfg.DialOptions = append(cfg.DialOptions, grpc.WithTransportCredentials(cfg.Metrics.GRPCCredentials))
creds := cfg.Metrics.GRPCCredentials
cfg.Metrics.GRPCCredentials = creds
cfg.DialOptions = append(cfg.DialOptions, grpc.WithTransportCredentials(creds))
} else if cfg.Metrics.Insecure {
cfg.DialOptions = append(cfg.DialOptions, grpc.WithTransportCredentials(insecure.NewCredentials()))
creds := insecure.NewCredentials()
cfg.Metrics.GRPCCredentials = creds
cfg.DialOptions = append(cfg.DialOptions, grpc.WithTransportCredentials(creds))
} else {
// Default to using the host's root CA.
creds := credentials.NewTLS(nil)
Expand Down
1 change: 1 addition & 0 deletions exporters/otlp/otlpmetric/internal/oconf/options_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ func TestConfigs(t *testing.T) {
assert.Equal(t, oconf.NoCompression, c.Metrics.Compression)
assert.Equal(t, map[string]string(nil), c.Metrics.Headers)
assert.Equal(t, 10*time.Second, c.Metrics.Timeout)
assert.True(t, c.Metrics.Insecure)
},
},

Expand Down
7 changes: 3 additions & 4 deletions exporters/otlp/otlpmetric/otlpmetrichttp/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,9 @@ func TestConfig(t *testing.T) {
assert.Len(t, rCh, 0, "failed HTTP responses did not occur")
})

t.Run("WithURLPath", func(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewer(s) – This test is a dupe, see L151 below.

path := "/prefix/v2/metrics"
ePt := fmt.Sprintf("http://localhost:0%s", path)
exp, coll := factoryFunc(ePt, nil, WithURLPath(path))
t.Run("WithInsecure", func(t *testing.T) {
ePt := "http://localhost:0"
exp, coll := factoryFunc(ePt, nil, WithInsecure())
ctx := context.Background()
t.Cleanup(func() { require.NoError(t, coll.Shutdown(ctx)) })
t.Cleanup(func() { require.NoError(t, exp.Shutdown(ctx)) })
Expand Down
4 changes: 2 additions & 2 deletions exporters/otlp/otlptrace/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,12 @@ specification](https://github.com/open-telemetry/opentelemetry-specification/blo

| Environment variable | Option | Default value |
| ------------------------------------------------------------------------ |------------------------------ | -------------------------------------------------------- |
| `OTEL_EXPORTER_OTLP_ENDPOINT` `OTEL_EXPORTER_OTLP_TRACES_ENDPOINT` | `WithEndpoint` `WithInsecure` | `https://localhost:4317` or `https://localhost:4318`[^1] |
| `OTEL_EXPORTER_OTLP_ENDPOINT` `OTEL_EXPORTER_OTLP_TRACES_ENDPOINT` | `WithEndpoint` `WithInsecure` | `http://localhost:4317` or `http://localhost:4318`[^1] |
| `OTEL_EXPORTER_OTLP_CERTIFICATE` `OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE` | `WithTLSClientConfig` | |
| `OTEL_EXPORTER_OTLP_HEADERS` `OTEL_EXPORTER_OTLP_TRACES_HEADERS` | `WithHeaders` | |
| `OTEL_EXPORTER_OTLP_COMPRESSION` `OTEL_EXPORTER_OTLP_TRACES_COMPRESSION` | `WithCompression` | |
| `OTEL_EXPORTER_OTLP_TIMEOUT` `OTEL_EXPORTER_OTLP_TRACES_TIMEOUT` | `WithTimeout` | `10s` |

[^1]: The gRPC client defaults to `https://localhost:4317` and the HTTP client `https://localhost:4318`.
[^1]: The gRPC client defaults to `http://localhost:4317` and the HTTP client `http://localhost:4318`.

Configuration using options have precedence over the environment variables.
22 changes: 19 additions & 3 deletions exporters/otlp/otlptrace/internal/otlpconfig/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,20 @@ func NewHTTPConfig(opts ...HTTPOption) Config {
URLPath: DefaultTracesPath,
Compression: NoCompression,
Timeout: DefaultTimeout,
Insecure: true,
},
RetryConfig: retry.DefaultConfig,
}

cfg = ApplyHTTPEnvConfigs(cfg)
for _, opt := range opts {
cfg = opt.ApplyHTTPOption(cfg)
}

if cfg.Traces.TLSCfg != nil {
cfg.Traces.Insecure = false
}

cfg.Traces.URLPath = internal.CleanPath(cfg.Traces.URLPath, DefaultTracesPath)
return cfg
}
Expand All @@ -96,6 +103,7 @@ func NewGRPCConfig(opts ...GRPCOption) Config {
URLPath: DefaultTracesPath,
Compression: NoCompression,
Timeout: DefaultTimeout,
Insecure: true,
},
RetryConfig: retry.DefaultConfig,
DialOptions: []grpc.DialOption{grpc.WithUserAgent(otinternal.GetUserAgentHeader())},
Expand All @@ -105,14 +113,22 @@ func NewGRPCConfig(opts ...GRPCOption) Config {
cfg = opt.ApplyGRPCOption(cfg)
}

if cfg.Traces.TLSCfg != nil {
cfg.Traces.Insecure = false
}

if cfg.ServiceConfig != "" {
cfg.DialOptions = append(cfg.DialOptions, grpc.WithDefaultServiceConfig(cfg.ServiceConfig))
}
// Priroritize GRPCCredentials over Insecure (passing both is an error).
// Prioritize GRPCCredentials over Insecure (passing both is an error).
if cfg.Traces.GRPCCredentials != nil {
cfg.DialOptions = append(cfg.DialOptions, grpc.WithTransportCredentials(cfg.Traces.GRPCCredentials))
creds := cfg.Traces.GRPCCredentials
cfg.Traces.GRPCCredentials = creds
cfg.DialOptions = append(cfg.DialOptions, grpc.WithTransportCredentials(creds))
} else if cfg.Traces.Insecure {
cfg.DialOptions = append(cfg.DialOptions, grpc.WithTransportCredentials(insecure.NewCredentials()))
creds := insecure.NewCredentials()
cfg.Traces.GRPCCredentials = creds
cfg.DialOptions = append(cfg.DialOptions, grpc.WithTransportCredentials(creds))
} else {
// Default to using the host's root CA.
creds := credentials.NewTLS(nil)
Expand Down
1 change: 1 addition & 0 deletions exporters/otlp/otlptrace/internal/otlpconfig/options_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ func TestConfigs(t *testing.T) {
assert.Equal(t, otlpconfig.NoCompression, c.Traces.Compression)
assert.Equal(t, map[string]string(nil), c.Traces.Headers)
assert.Equal(t, 10*time.Second, c.Traces.Timeout)
assert.True(t, c.Traces.Insecure)
},
},

Expand Down
6 changes: 6 additions & 0 deletions exporters/otlp/otlptrace/otlptracehttp/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,12 @@ func TestEndToEnd(t *testing.T) {
ExpectedHeaders: customUserAgentHeader,
},
},
{
name: "with insecure",
opts: []otlptracehttp.Option{
otlptracehttp.WithInsecure(),
},
},
}

for _, tc := range tests {
Expand Down