Add spec for AlwaysRecord sampler #4699
Conversation
jmacd
left a comment
jmacd
left a comment
There was a problem hiding this comment.
Looks good to me. As we discussed in Sampling SIG, it will help readers to be reminded somewhere in this text that Exporters are already expected to drop these spans, all we're doing is making it easy to setup non-sampling recorded spans by default.
|
Apologies for the delay, pushed a new revision. Please help to run the workflows again, thanks! |
|
Perhaps @jmacd could you help to merge? Thanks |
|
Ping @majanjua-amzn (we need a last few details to iron out, but very close to merge this). |
|
@carlosalberto, could you help to run the workflows again and approve if all looks good now? Thanks |
|
Question in general: Should these be added to the core or contrib repo? For example:
|
majanjua-amzn
force-pushed
the
main
branch
2 times, most recently
from
3a01a76 to
497fea4
Compare
|
@cijothomas finished another revision, please take a look when you can Also, if anyone has input on the previous comment regarding where to add this feature, please advise:
|
|
Great, made a final revision with respect to the last comment. If reviewers could re-approve if all looks good and merge this PR, I can also start finalizing my PRs to each other repo and start contributing the feature to OTel core! Thanks in advance |
|
Perhaps @jmacd and @carlosalberto can help provide reviews again, as you folks were early reviewers the first time? PRs are awaiting the merge of this one in Java, Python, JS, and .NET: Java: open-telemetry/opentelemetry-java#7877 |
|
@cijothomas could you help re-approve and merge now? Thanks! |
|
@MrAlias good callouts, fixed! |
@majanjua-amzn have you forgotten to push a commit or two? None of my previous comments have been addressed. |
|
@MrAlias forgot to push, sorry for the double ping! |
|
Can anyone help to merge now? Thanks! |
### Context - Make the W3C randomness flag required. ([#4761](#4761)) ### Traces - Deprecate Zipkin exporter document and make exporter implementation optional. ([#4715](#4715)) - Add spec for `AlwaysRecord` sampler ([#4699](#4699)) ### Metrics - Stabilize `Enabled` API for synchronous instruments. ([#4746](#4746)) - Allow instrument `Enabled` implementation to have additional optimizations and features. ([#4747](#4747)) ### Logs - Stabilize `LogRecordProcessor.Enabled`. ([#4717](#4717)) ### SDK Configuration - Clarifies that guidance related to boolean environment variables is not applicable to other configuration interfaces. ([#4723](#4723)) --------- Co-authored-by: Armin Ruech <7052238+arminru@users.noreply.github.com>
9 participants
Fixes #4698
Changes
Adds the spec for a new AlwaysRecord sampler being proposed in #4698
Notes
Solutions to this problem in multiple supported languages can already be found below:
Checklist
CHANGELOG.mdfile updated for non-trivial changes