Skip to content

Rename oltp_endpoint to otlp_endpoint to match opentelemetry spec and lib name #5068

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

Merged
merged 10 commits into from
Jan 27, 2023
Merged

Rename oltp_endpoint to otlp_endpoint to match opentelemetry spec and lib name #5068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
38b09b9
rename oltp_endpoint to otlp_endpoint to be consistent with library n…
sharkymcdongles Dec 26, 2022
c360051
update changelog
sharkymcdongles Dec 26, 2022
5d0a1e1
change BUGFIX to CHANGE
sharkymcdongles Dec 26, 2022
6b5e95c
deprecate instead of remove
sharkymcdongles Jan 9, 2023
c50f47a
fix errors because I forgot this is golang and not python lol
sharkymcdongles Jan 10, 2023
6b5b269
improve documentation comment
sharkymcdongles Jan 15, 2023
85fde37
remove newline
sharkymcdongles Jan 16, 2023
c9181ab
hide deprecated config
sharkymcdongles Jan 17, 2023
5ccf7db
remove hidden
sharkymcdongles Jan 18, 2023
080e980
Merge branch 'master' into master
sharkymcdongles Jan 27, 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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## master / unreleased
* [CHANGE] Alertmanager: Local file disclosure vulnerability in OpsGenie configuration has been fixed. #5045
* [CHANGE] Rename oltp_endpoint to otlp_endpoint to match opentelemetry spec and lib name. #5067
* [CHANGE] Distributor/Ingester: Log warn level on push requests when they have status code 4xx. Do not log if status is 429. #5103
* [ENHANCEMENT] Update Go version to 1.19.3. #4988
* [ENHANCEMENT] Querier: limit series query to only ingesters if `start` param is not specified. #4976
Expand Down
4 changes: 2 additions & 2 deletions docs/configuration/config-file-reference.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4158,8 +4158,8 @@ The `tracing_config` configures backends cortex uses.

otel:
# otl collector endpoint that the driver will use to send spans.
# CLI flag: -tracing.otel.oltp-endpoint
[oltp_endpoint: <string> | default = ""]
# CLI flag: -tracing.otel.otlp-endpoint
[otlp_endpoint: <string> | default = ""]

# enhance/modify traces/propagators for specific exporter. If empty, OTEL
# defaults will apply. Supported values are: `awsxray.`
Expand Down
26 changes: 21 additions & 5 deletions pkg/tracing/tracing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ type Config struct {
}

type Otel struct {
OltpEndpoint string `yaml:"oltp_endpoint" json:"oltp_endpoint"`
OltpEndpoint string `yaml:"oltp_endpoint" json:"oltp_endpoint" doc:"hidden"`
OtlpEndpoint string `yaml:"otlp_endpoint" json:"otlp_endpoint"`
ExporterType string `yaml:"exporter_type" json:"exporter_type"`
SampleRatio float64 `yaml:"sample_ratio" json:"sample_ratio"`
TLSEnabled bool `yaml:"tls_enabled"`
Expand All @@ -51,7 +52,8 @@ func (c *Config) RegisterFlags(f *flag.FlagSet) {
p := "tracing"
f.StringVar(&c.Type, p+".type", JaegerType, "Tracing type. OTEL and JAEGER are currently supported. For jaeger `JAEGER_AGENT_HOST` environment variable should also be set. See: https://cortexmetrics.io/docs/guides/tracing .")
f.Float64Var(&c.Otel.SampleRatio, p+".otel.sample-ratio", 0.001, "Fraction of traces to be sampled. Fractions >= 1 means sampling if off and everything is traced.")
f.StringVar(&c.Otel.OltpEndpoint, p+".otel.oltp-endpoint", "", "otl collector endpoint that the driver will use to send spans.")
f.StringVar(&c.Otel.OltpEndpoint, p+".otel.oltp-endpoint", "", "DEPRECATED: use otel.otlp-endpoint instead.")
f.StringVar(&c.Otel.OtlpEndpoint, p+".otel.otlp-endpoint", "", "otl collector endpoint that the driver will use to send spans.")
f.StringVar(&c.Otel.ExporterType, p+".otel.exporter-type", "", "enhance/modify traces/propagators for specific exporter. If empty, OTEL defaults will apply. Supported values are: `awsxray.`")
f.BoolVar(&c.Otel.TLSEnabled, p+".otel.tls-enabled", c.Otel.TLSEnabled, "Enable TLS in the GRPC client. This flag needs to be enabled when any other TLS flag is set. If set to false, insecure connection to gRPC server will be used.")
c.Otel.TLS.RegisterFlagsWithPrefix(p+".otel.tls", f)
Expand All @@ -60,8 +62,11 @@ func (c *Config) RegisterFlags(f *flag.FlagSet) {
func (c *Config) Validate() error {
switch strings.ToLower(c.Type) {
case OtelType:
if c.Otel.OltpEndpoint == "" {
return errors.New("oltp-endpoint must be defined when using otel exporter")
if (c.Otel.OtlpEndpoint == "") && (c.Otel.OltpEndpoint == "") {
return errors.New("otlp-endpoint must be defined when using otel exporter")
}
if len(c.Otel.OltpEndpoint) > 0 {
level.Warn(util_log.Logger).Log("DEPRECATED: otel.oltp-endpoint is deprecated. User otel.otlp-endpoint instead.")
}
}

Expand All @@ -83,8 +88,19 @@ func SetupTracing(ctx context.Context, name string, c Config) (func(context.Cont
case OtelType:
util_log.Logger.Log("msg", "creating otel exporter")

if (len(c.Otel.OtlpEndpoint) > 0) && (len(c.Otel.OltpEndpoint) > 0) {
level.Warn(util_log.Logger).Log("msg", "DEPRECATED: otel.otlp and otel.oltp both set, using otel.otlp because otel.oltp is deprecated")
}

options := []otlptracegrpc.Option{
otlptracegrpc.WithEndpoint(c.Otel.OltpEndpoint),
otlptracegrpc.WithEndpoint(c.Otel.OtlpEndpoint),
}

if (c.Otel.OtlpEndpoint == "") && (len(c.Otel.OltpEndpoint) > 0) {
level.Warn(util_log.Logger).Log("msg", "DEPRECATED: otel.oltp is deprecated use otel.otlp")
options = []otlptracegrpc.Option{
otlptracegrpc.WithEndpoint(c.Otel.OltpEndpoint),
}
}
Comment on lines +99 to 104
Copy link
Member

Choose a reason for hiding this comment

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

If both are set OtlpEndpoint should take precedence (this will happen when we are migrating from the old config to the new one)

Something like:

if (len(c.Otel.OtlpEndpoint) > 0) && (len(c.Otel.OltpEndpoint) > 0) {
	level.Warn(util_log.Logger).Log("msg", "DEPRECATED: otel.otlp and otel.oltp both set, using otel.otlp because otel.oltp is deprecated")
}

otlpEndpoint := c.Otel.OltpEndpoint

if len(otlpEndpoint) == 0 {
    otlpEndpoint = c.Otel.OltpEndpoint
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already did that here: 91

Logic is:

If both are set log warning that both are set and otel.oltp will be removed and set options to use OtlpEndpoint.

If only OltpEndpoint is set then use it and warn as well.

Then once it is fully removed then both the if from 91 and 99 can be removed.

Maybe there is a cleaner way than this if else condition though not sure.


if c.Otel.TLSEnabled {
Expand Down