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

Introduce sampling package as reference implementation for OTEP 235 #29720

Merged
merged 42 commits into from
Jan 31, 2024

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Dec 8, 2023

Description: This is the pkg/sampling portion of of #24811.

Link to tracking Issue:
#29738
open-telemetry/opentelemetry-specification#1413

Testing: Complete.

Documentation: New README added.

@jmacd jmacd marked this pull request as ready for review December 8, 2023 22:31
@jmacd jmacd requested review from a team and dmitryax December 8, 2023 22:31
@github-actions github-actions bot added the processor/probabilisticsampler Probabilistic Sampler processor label Dec 8, 2023
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks for splitting up the PR @jmacd, this is much easier to review. Will add some more comments, just some quick things to fix noted so far

processor/probabilisticsamplerprocessor/go.mod Outdated Show resolved Hide resolved
.github/CODEOWNERS Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor Author

jmacd commented Dec 11, 2023

@kentquirk please confirm willingness to co-own this code :-)

@jpkrohling
Copy link
Member

I'd like to review this, but I'm unable to do so this year. I'm back on Jan 15, please ping me again on Jan 18 if I haven't reviewed this here by then.

pkg/sampling/doc.go Outdated Show resolved Hide resolved
pkg/sampling/doc.go Outdated Show resolved Hide resolved
pkg/sampling/common.go Show resolved Hide resolved
pkg/sampling/common.go Outdated Show resolved Hide resolved
pkg/sampling/common.go Outdated Show resolved Hide resolved
pkg/sampling/encoding_test.go Show resolved Hide resolved
pkg/sampling/encoding_test.go Show resolved Hide resolved
pkg/sampling/oteltracestate.go Outdated Show resolved Hide resolved
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

I haven't looked at the other PR that utilizes this package, but i'm curious if the surface area being exposed in this package could be reduced

pkg/sampling/oteltracestate.go Outdated Show resolved Hide resolved
Copy link
Member

@kentquirk kentquirk left a comment

Choose a reason for hiding this comment

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

Leaving a bunch more comments since it's changing out from under me while I try to make them.

pkg/sampling/oteltracestate.go Outdated Show resolved Hide resolved
pkg/sampling/doc.go Outdated Show resolved Hide resolved
pkg/sampling/encoding_test.go Outdated Show resolved Hide resolved
pkg/sampling/encoding_test.go Outdated Show resolved Hide resolved
pkg/sampling/encoding_test.go Outdated Show resolved Hide resolved
pkg/sampling/threshold.go Outdated Show resolved Hide resolved
pkg/sampling/randomness.go Show resolved Hide resolved
pkg/sampling/encoding_test.go Show resolved Hide resolved
pkg/sampling/oteltracestate_test.go Show resolved Hide resolved
Co-authored-by: Kent Quirk <kentquirk@gmail.com>
@jmacd
Copy link
Contributor Author

jmacd commented Jan 26, 2024

@kentquirk thanks! I'm going to rest on this for a while, with the couple of unresolved comments you left standing. I'm willing to come back to the error handling in Serialize later if possible, but would like to resolve the HasField() and Field() accessor questions.

Other reviewers, lots of progress has been made, and it's very close.

pkg/sampling/doc.go Outdated Show resolved Hide resolved
@codeboten
Copy link
Contributor

@kentquirk @atoulme please review and approve if your comments have been addressed

jmacd and others added 3 commits January 30, 2024 11:41
Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de>
Copy link
Member

@kentquirk kentquirk left a comment

Choose a reason for hiding this comment

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

I'm now happy with this (and reiterate my agreement to be a codeowner).

@jmacd
Copy link
Contributor Author

jmacd commented Jan 31, 2024

FYI OTEP 235 merged. I think this is ready to merge, too. 😀

Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

LGTM

@jpkrohling jpkrohling merged commit 2cebf1f into open-telemetry:main Jan 31, 2024
85 checks passed
@github-actions github-actions bot added this to the next release milestone Jan 31, 2024
@jmacd jmacd deleted the jmacd/pkgsampl branch January 31, 2024 17:29
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this pull request Feb 1, 2024
…pen-telemetry#29720)

**Description:** This is the `pkg/sampling` portion of of
open-telemetry#24811.

**Link to tracking Issue:** 
open-telemetry#29738

open-telemetry/opentelemetry-specification#1413

**Testing:** Complete.

**Documentation:** New README added.

---------

Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de>
Co-authored-by: Kent Quirk <kentquirk@gmail.com>
@jpkrohling jpkrohling mentioned this pull request Feb 14, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
processor/probabilisticsampler Probabilistic Sampler processor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants