Skip to content
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

Change the default sampler to ParentOrElse(AlwaysOn) #750

Conversation

MitchellDumovic
Copy link
Contributor

Discussed at the Spec SIG this morning 7/28. Resolves #728

@MitchellDumovic MitchellDumovic requested review from a team July 28, 2020 21:41
@MitchellDumovic MitchellDumovic force-pushed the mitchell/change-default-sampler branch from 1ce0aad to 458a0d5 Compare July 28, 2020 21:42
@dyladan
Copy link
Member

dyladan commented Jul 28, 2020

Because it is the default, you may want to move it to the top of the list. I would also consider adding a new heading "Default Sampler" that specifically calls out that this is the default sampler so it isn't hidden in a bullet list.

@MitchellDumovic
Copy link
Contributor Author

MitchellDumovic commented Jul 28, 2020

@dyladan I pushed some changes that hopefully address the problem here. I have a slight preference for not leading with ParentOrElse as it is a more complex sampler type and the default ParentOrElse(AlwaysOn) references both ParentOrElse and AlwaysOn.

It felt more right to me to include the default header right next to the documentation of the built-in sampler types rather than adding a new higher level header, but again this is a small preference so happy to split out a separate header as well if you think that is more clear and makes the default more obvious.

@@ -94,6 +94,8 @@ be displayed on debug pages or in the logs. Example:
Description MUST NOT change over time and caller can cache the returned value.

### Built-in samplers
OpenTelemetry supports a number of built-in samplers to choose from.
The default sampler if one is not specified is `ParentOrElse(AlwaysOn)`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The default sampler if one is not specified is `ParentOrElse(AlwaysOn)`.
The default sampler if is `ParentOrElse(AlwaysOn)`.

@Oberon00
Copy link
Member

I'm OK with this change, but IMHO it was merged far too fast. #745 suggests to wait two business days, but even without that, I think merging a non-trivial change on the same day it was proposed is unfair.

@yurishkuro
Copy link
Member

@Oberon00 my bad. Is there some bot we can install that would block merge for a predefined "public comment" period? Similar to how WIP bot blocks the merge.

carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
)

* Change the default sampler to ParentOrElse(AlwaysOn)

* addressed some review comments

* more review feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revisit Default Sampler and Interaction With TraceFlags
6 participants