-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Update OpenTelemetry Tracing version status #13808
Conversation
Overall Looks good to me, please add an entry into 3.6 CHANGELOG. We also need an agreement as discussed in issues/13775. |
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.
Before marking distributed tracing as a stable feature we need to agree on expectations of how stable tracing for etcd should look like. Does it work? Do we have enough tests? Is tracking used withing Etcd codebase?
Would be good to discuss it with the K8s tracing owner @dashpole
An integration test similar to kubernetes/kubernetes#103234 would be useful. @hanyuancheung have you gotten any user feedback from trying to use the feature, or know of successful use anywhere? |
The latest release is 1.5.0. |
Thanks for the info. etcd needs to add similar integration test. Just raised an issue to track this. |
I agree. This PR should focus on updating otel tracing version to the newest one, and I've noticed @ahrtr has opened the issue to track the addition integration test for etcd tracing. |
I'll also work on #13814 after this one finished, to make sure the feature is useful with the integration test 😄 Thx for ur suggestion! |
server/embed/config.go
Outdated
// Can only be used if ExperimentalEnableDistributedTracing is true. | ||
ExperimentalDistributedTracingServiceName string `json:"experimental-distributed-tracing-service-name"` | ||
// ExperimentalDistributedTracingServiceInstanceID is the ID key of the service. | ||
// EnableDistributedTracing indicates if tracing using OpenTelemetry is enabled. |
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.
Let's have a separate PR to stabilize distributing tracing feature. We still have some work before stabilization.
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 get it. So this PR should just work on updating the version of OpenTelemetry right?
If so, I'll cancel the changes of all stabilization work ~
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.
Yes, we can update OpenTelemetry now and decide separably to stabilize the tracing.
Co-authored-by: Marek Siarkowicz <marek.siarkowicz@protonmail.com>
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.
Codecov Report
@@ Coverage Diff @@
## main #13808 +/- ##
==========================================
- Coverage 72.72% 72.07% -0.65%
==========================================
Files 467 467
Lines 38280 38280
==========================================
- Hits 27839 27591 -248
- Misses 8662 8883 +221
- Partials 1779 1806 +27
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
The Could you rebase this PR and bump the |
Thanks @houseme for the confirmation. |
Follow: #12929
Fixs: #12460
Noticed that OpenTelemetry has released some stable versions as well as new features. Now update its version, and remove the
experimental
status in tracing config.