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

[chore] Add note to GroupbyTraceprocessor when working with tailsampling #26997

Merged
merged 5 commits into from
Sep 20, 2023

Conversation

jiekun
Copy link
Member

@jiekun jiekun commented Sep 19, 2023

Description:

I'm proposing a small note in both tailsamplingprocessor and groupbytrace processor.

In order to perform a tail-based sampling, spans need to be grouped by trace_id.
We have a groupbytrace processor with README describes:

This processor should be used whenever a processor requires grouped traces to make decisions,
such as a tail-based sampler or ...

The description is correct since it said "(a customized) tail-based sampler", not the tailsamplingprocessor in this repo. But some people are still misdirected and think it's necessary to setup groupby processor before tailsampling processor.

Link to tracking Issue:
N/A

Testing:
N/A

Documentation:
The PR only change the README of both groupbytrace processor and tailsampling processor.

@jiekun jiekun requested review from jpkrohling and a team as code owners September 19, 2023 07:40
@github-actions github-actions bot added the processor/groupbytrace Group By Trace processor label Sep 19, 2023
@github-actions github-actions bot added the processor/tailsampling Tail sampling processor label Sep 19, 2023
@jiekun jiekun changed the title Add note to GroupbyTraceprocessor when working with tailsampling [chore] Add note to GroupbyTraceprocessor when working with tailsampling Sep 19, 2023
Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

LGTM

@songy23 songy23 added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Sep 19, 2023
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Thank you!

@jpkrohling jpkrohling merged commit c47ff80 into open-telemetry:main Sep 20, 2023
95 of 96 checks passed
@github-actions github-actions bot added this to the next release milestone Sep 20, 2023
@jiekun jiekun deleted the patch-1 branch September 20, 2023 09:26
jmsnll pushed a commit to jmsnll/opentelemetry-collector-contrib that referenced this pull request Nov 12, 2023
…ing (open-telemetry#26997)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
I'm proposing a small note in both tailsamplingprocessor and
groupbytrace processor.

In order to perform a tail-based sampling, spans need to be grouped by
`trace_id`.
We have a groupbytrace processor with README describes:
> This processor should be used whenever a processor requires grouped
traces to make decisions,
such as a tail-based sampler or ...

The description is correct since it said "(a customized) tail-based
sampler", not the `tailsamplingprocessor` in this repo. But some people
are still misdirected and think it's necessary to setup groupby
processor before tailsampling processor.

**Link to tracking Issue:** <Issue number if applicable>
N/A

**Testing:** <Describe what testing was performed and which tests were
added.>
N/A

**Documentation:** <Describe the documentation added.>
The PR only change the README of both groupbytrace processor and
tailsampling processor.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
processor/groupbytrace Group By Trace processor processor/tailsampling Tail sampling processor Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants