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

[pkg/ottl]Add trace_state.adjusted_count path to span context #36789

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

lahsivjar
Copy link
Member

@lahsivjar lahsivjar commented Dec 11, 2024

Description

Introduces trace_state.adjusted_count path which calculates adjusted count for an OTTL span.

Link to tracking issue

Fixes #36572
Alternative to #36573

Testing

Added unit tests

Documentation

@lahsivjar
Copy link
Member Author

Sorry about the review notifications folks, I was not expecting a new draft PR to add reviewers 😓

@lahsivjar
Copy link
Member Author

Bringing the PR out of draft state after getting confirmation on the approach in this comment.

@lahsivjar lahsivjar marked this pull request as ready for review December 16, 2024 19:57
@lahsivjar lahsivjar requested a review from a team as a code owner December 16, 2024 19:57
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Overall this looks okay to me, but I'd like someone more familiar with trace sampling to take a look. @kentquirk would you be willing to review this one?

.chloggen/add-adjusted-count-path.yaml Outdated Show resolved Hide resolved
pkg/ottl/contexts/internal/span.go Show resolved Hide resolved
@lahsivjar
Copy link
Member Author

👋 folks, the conflicts in the PR are trivial and will continue to happen with new version ups. To avoid repetitive task, I plan to fix the conflicts once the PR is approved but let me know if you want to have that fixed before reviewing - I can do that too.

@evan-bradley
Copy link
Contributor

@kentquirk If you have a minute, could you please review this?

otTraceState := w3cTraceState.OTelValue()
if otTraceState == nil {
// If otel trace state is missing, default to 1
return float64(1), nil
Copy link
Member

Choose a reason for hiding this comment

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

Is this defined by the sampling spec as a default value? If not we should probably error. It feels like accessing the trace state with an unset "key" is an issue.

@braydonk braydonk removed their request for review January 9, 2025 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New AdjustedCount converter function
4 participants