-
Notifications
You must be signed in to change notification settings - Fork 83
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.
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
feat: Add support for tracing for http addon #1021
base: main
Are you sure you want to change the base?
feat: Add support for tracing for http addon #1021
Conversation
f945b23
to
1eb19da
Compare
f64c6dc
to
480050e
Compare
- name: KEDA_HTTP_TRACE_ENDPOINT | ||
value: "opentelemetry-collector.open-telemetry-system:4318" | ||
- name: KEDA_HTTP_TRACE_INSECURE | ||
value: "true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't noticed it with the first PR was created, but we should respect the OTEL spec for env vars: https://opentelemetry.io/docs/specs/otel/protocol/exporter/
Could you change the new envs to respect (or use) the SDK envs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored to using the OTEL SDK envs instead
c95f18e
to
eb64c18
Compare
interceptor/config/tracing.go
Outdated
type Tracing struct { | ||
// States whether tracing should be enabled, False by default | ||
Enabled bool `envconfig:"KEDA_HTTP_TRACING_ENABLED" default:"false"` | ||
// Sets what tracing export to use, must be one of: console,otlphttp | ||
Exporter string `envconfig:"KEDA_HTTP_TRACE_EXPORTER" default:"console"` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are going to rename other envs to be aligned with the spec, so we have to change these 2 too
What about
type Tracing struct { | |
// States whether tracing should be enabled, False by default | |
Enabled bool `envconfig:"KEDA_HTTP_TRACING_ENABLED" default:"false"` | |
// Sets what tracing export to use, must be one of: console,otlphttp | |
Exporter string `envconfig:"KEDA_HTTP_TRACE_EXPORTER" default:"console"` | |
} | |
type Tracing struct { | |
// States whether tracing should be enabled, False by default | |
Enabled bool `envconfig:"OTEL_EXPORTER_OTLP_TRACES_ENABLED" default:"false"` | |
// Sets what tracing export to use, must be one of: console,otlphttp | |
Exporter string `envconfig:"OTEL_EXPORTER_OTLP_PROTOCOL" default:"console"` | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored as per comment
interceptor/tracing/tracing.go
Outdated
switch strings.ToLower(tCfg.Exporter) { | ||
case "console": | ||
return stdouttrace.New() | ||
case "otlphttp": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this should be http/protobuf
based on docs: https://opentelemetry.io/docs/specs/otel/protocol/exporter/#specify-protocol
Does it make sense supporting also grpc
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added both supports
@bhussain91 any update on this please? |
Signed-off-by: Bilal Hussain <[email protected]>
eb64c18
to
b093281
Compare
Signed-off-by: Bilal Hussain [email protected]
Provide a description of what has been changed
The change is to add tracing capability into the keda http addon using opentelemetry. The changes allows the add on to generate and export spans to console (development) or otel gRPC endpoint. Further information on how to configure tracing can be found in the docs
Checklist
README.md
docs/
directoryFixes #