feat(tracing): migrate from OpenTracing/Jaeger to OpenTelemetry#5456
feat(tracing): migrate from OpenTracing/Jaeger to OpenTelemetry#5456martinconic wants to merge 3 commits into
Conversation
martinconic
requested review from
acud,
akrem-chabchoub,
gacevicljubisa,
janos and
sbackend123
and removed request for
gacevicljubisa
sbackend123
left a comment
sbackend123
left a comment
There was a problem hiding this comment.
I would explicitly call out in the PR description and in the docs which configuration flags were added / removed and that the tracing endpoint semantics changed. Without that, anyone still on the old configuration may be caught off guard.
| SETUP_CONTRACT_IMAGE_TAG: "0.9.4" | ||
| BEELOCAL_BRANCH: "main" | ||
| BEEKEEPER_BRANCH: "master" | ||
| BEEKEEPER_BRANCH: "feat/bee-otlp-tracing-config" |
There was a problem hiding this comment.
Before merge do not forget to switch beekeeper on master again
| if ratio <= 0 { | ||
| ratio = 1 | ||
| } |
There was a problem hiding this comment.
Why not allow 0? It can serve as a switch off... Flag is already setting it by default to 1.
| optionNameTracingEnabled = "tracing-enable" | ||
| optionNameTracingEndpoint = "tracing-endpoint" | ||
| optionNameTracingHost = "tracing-host" | ||
| optionNameTracingPort = "tracing-port" | ||
| optionNameTracingOTLPEndpoint = "tracing-otlp-endpoint" | ||
| optionNameTracingOTLPInsecure = "tracing-otlp-insecure" | ||
| optionNameTracingSamplingRatio = "tracing-sampling-ratio" | ||
| optionNameTracingServiceName = "tracing-service-name" |
There was a problem hiding this comment.
If we are changing this configuration, we can support nested flags as well for tracing:
tracing:
enable: true
otlp-endpoint: ...
gacevicljubisa
left a comment
gacevicljubisa
left a comment
There was a problem hiding this comment.
I would explicitly call out in the PR description and in the docs which configuration flags were added / removed and that the tracing endpoint semantics changed. Without that, anyone still on the old configuration may be caught off guard.
Yes, I agree. Updating the node with old config will result with "unknown flag" error.
Also, this is breaking change for the tracing for the node operators who are using tracing, they will need to switch to OTLP receiver, instead of jaeger UDP agent.
Additionally, OTel has a second exporter, otlptracegrpc. It might be worth to expose a tracing-otlp-protocol: http|grpc flag (defaulting to http), so operators can choose.
3 participants
Checklist
Description
Migrates bee's tracing stack from the archived OpenTracing API and
deprecated jaeger-client-go to the OpenTelemetry Go SDK with an
OTLP/HTTP exporter. The wrapper at
pkg/tracingkeeps a familiar APIshape so the call-site sweep is bounded to ~8 production files plus
the test suite.
Closes #5010.
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):
AI Disclosure