-
Notifications
You must be signed in to change notification settings - Fork 224
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
chore: deprecate jaeger #2675
chore: deprecate jaeger #2675
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2675 +/- ##
==========================================
- Coverage 73.02% 72.99% -0.03%
==========================================
Files 85 85
Lines 8326 8325 -1
==========================================
- Hits 6080 6077 -3
- Misses 1869 1871 +2
Partials 377 377 ☔ View full report in Codecov by Sentry. |
internal/config/tracing.go
Outdated
@@ -22,11 +22,10 @@ type TracingConfig struct { | |||
func (c *TracingConfig) setDefaults(v *viper.Viper) error { | |||
v.SetDefault("tracing", map[string]any{ | |||
"enabled": false, | |||
"exporter": TracingJaeger, | |||
"exporter": TracingOTLP, |
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.
Questioning this incase it wasn't considered:
Should we be simultaneously deprecating and changing the default behaviour like this?
Just wondering if there might be folks out there with solely:
tracing:
enabled: true
That might just be relying on it talking to jaeger.
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.
good call. I also added a tracing
section to telemetry so we can hopefully get more data about when its safe to fully remove
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.
Change looks good. Open question about whether changing the default at the same time makes sense.
Fixes: #2650