-
Notifications
You must be signed in to change notification settings - Fork 893
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
[For 1.0] Add WARNING for TraceIdRatioBased algorithm stability. #1414
[For 1.0] Add WARNING for TraceIdRatioBased algorithm stability. #1414
Conversation
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.
Thank you!
Co-authored-by: Armin Ruech <[email protected]>
specification/trace/sdk.md
Outdated
sampling rate than the frontend system, this way all frontend traces will | ||
still be sampled and extra traces will be sampled on the backend only. | ||
* **WARNING:** Since the exact algorithm is not specified yet (see TODO above), | ||
there will probably be be breaking changes to it in any language SDK once it is. |
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.
This language makes it sound like we will be required to bump to a 2.0.0 version when this is specified. Is that the intent?
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.
The intent was exactly to avoid that. Maybe there is a better word than "breaking" here? Or "if you rely on the algorithm, your code will break"?
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.
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 am confused about the description of the PR, ParentBased seems simple and stable
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.
This is a pragmatic choice that avoids churning APIs/SDKs just before the release, with a fairly limited downside risk for future releases.
@yurishkuro are you ok to move with this option then? |
sgtm |
…n-telemetry#1414) * [For 1.0] Add WARNING for ParentBased algorithm stability. * Update specification/trace/sdk.md Co-authored-by: Armin Ruech <[email protected]> * Remove non-working link to ParentBased. * Add back link with correct casing. * Reword. Co-authored-by: Armin Ruech <[email protected]> Co-authored-by: Bogdan Drutu <[email protected]>
Required for releasing 1.0 (#1373).
Alternative to PR #1412.
See #1372 (comment)
TraceIdRatioBased is only vaguely specified and has a TODO in it. We should call attention to the resulting unstable-ness of the algorithm.