Skip to content

add sampling support to span processors #200

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

Merged
merged 2 commits into from
Jan 8, 2025
Merged

Conversation

blakeroberts-wk
Copy link
Contributor

@blakeroberts-wk blakeroberts-wk commented Jan 7, 2025

Which problem is this PR solving?

This adds sampling support to the simple and batch span processors. As it stands now, even if a sampler is configured, the sampling decision does not affect whether the span is exported.

Fixes # N/A

Short description of the change

There are two functional changes:

  1. updating the batch and simple processors to check the sampling flag
  2. updating the trace helper functions to support taking a list of attributes to add at span start necessary for sampling on attributes

Additionally:

  1. an example program was added
  2. unit tests were updated
  3. exporter's forceFlush was deprecated because it's a kind of state dependent operation that should be managed by the processor that uses an exporter.
  4. non-functional changes to simplify the SpanContext class.

How Has This Been Tested?

The example program and unit tests sufficiently demonstrate functionality.

Checklist:

  • Unit tests have been added
  • Documentation has been updated

@blakeroberts-wk
Copy link
Contributor Author

QA +1 the example program outputs the following:

Span started: span-not-sampled
Span ended: span-not-sampled
Span started: span-sampled
Span ended: span-sampled
{traceId: 94f26b7841f0aa268128fd1f4e4f493c, parentId: 0000000000000000, name: span-sampled, id: ac98e8435956ac7e, timestamp: 1736286063230117000, duration: 2624000, flags: 01, state: , status: StatusCode.unset}
Shutting down

@todbachman-wf
Copy link
Member

todbachman-wf commented Jan 8, 2025

Question: I'm concerned about the long term ownership and maintainability of this code. This feels like a lot of very specific observability code to add to wdesk_sdk given our team doesn't own the observability story and doesn't/won't understand this code well enough to own it and maintain it long term. This has historically introduced challenges for our team as others have put code in wdesk_sdk because it's a convenient place to put it. Can we instead house this code in a repo that the Observability team owns and expose a public API that wdesk_sdk can then consume?

😆 I thought this PR was into wdesk_sdk, but it's not so I have no concerns. I'm gonna go get some more coffee now. ☕ ☕ ☕

@blakeroberts-wk
Copy link
Contributor Author

QA +1

@blakeroberts-wk
Copy link
Contributor Author

@Workiva/release-management-p

Copy link

@rmconsole-wf rmconsole-wf left a comment

Choose a reason for hiding this comment

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

+1 from RM

@btr-rmconsole-4 btr-rmconsole-4 bot merged commit 6ccd729 into master Jan 8, 2025
3 checks passed
@btr-rmconsole-4 btr-rmconsole-4 bot deleted the add-sampling branch January 8, 2025 22:33
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.

4 participants