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

Add new tail sampling processor policy: status_code #3754

Merged
merged 10 commits into from
Jun 24, 2021

Conversation

yvrhdn
Copy link
Contributor

@yvrhdn yvrhdn commented Jun 11, 2021

Description:
This adds a new policy to the tail sampling processor sampling traces based upon the status code of a span. Adding this policy allows you to sample all traces of which the status code matches a given status code.

Link to tracking Issue:

Testing:

Added unit tests similar to the other policies.

⚠️ I haven't been able to write proper tests because I can't figure out how to set the status code on spans manually. All the pdata data structures are closed off: fields are private and there is not setter for status code 🤷🏻 If there is a mechanism to still set it, please let me know and I'll update the tests!

Documentation:

I have updated the README.

@yvrhdn yvrhdn requested a review from jpkrohling as a code owner June 11, 2021 10:39
@yvrhdn yvrhdn requested a review from a team June 11, 2021 10:39
@yvrhdn
Copy link
Contributor Author

yvrhdn commented Jun 11, 2021

We won't be able to merge this PR and #3750 at the same time because of merge conflicts, but I can resolve any conflicts in whatever PR we merge second.

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.

I haven't been able to write proper tests because I can't figure out how to set the status code on spans manually. All the pdata data structures are closed off: fields are private and there is not setter for status code

Doesn't this one here work?

https://github.com/open-telemetry/opentelemetry-collector/blob/0d747472e565e6548ebfe42a3b3205d29d9e9065/consumer/pdata/traces.go#L150-L162

processor/tailsamplingprocessor/sampling/status_code.go Outdated Show resolved Hide resolved
@yvrhdn
Copy link
Contributor Author

yvrhdn commented Jun 11, 2021

Doesn't this one here work?

https://github.com/open-telemetry/opentelemetry-collector/blob/0d747472e565e6548ebfe42a3b3205d29d9e9065/consumer/pdata/traces.go#L150-L162

🤯 yeah that works as you'd expect, not sure how I missed it 😅

Koenraad Verheyden added 4 commits June 15, 2021 11:59
Move repeated boiler plate logic related to iterating through traces into a common set of functions.
@yvrhdn
Copy link
Contributor Author

yvrhdn commented Jun 15, 2021

Rebased on top of #3787

@yvrhdn
Copy link
Contributor Author

yvrhdn commented Jun 15, 2021

I'm going to pull this comment out of the review so it's easier to continue the discussion:

Don't we have status codes as attributes in the resource span attributes?

I'm not sure, I can't find anything that would say so. According to the spec the status is a separate field on a span (opentelemetry-specification/specification/trace/api.md#span).

I've been digging through the various resource attribtues, but haven't found anything related to the span status or status code. (opentelemetry-specification/specification/resource/semantic_conventions/README.md)

@yvrhdn
Copy link
Contributor Author

yvrhdn commented Jun 15, 2021

(I'm going to rebase this again after #3787 and #3788 get merged, this will clean up the commits)

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jun 23, 2021
@yvrhdn
Copy link
Contributor Author

yvrhdn commented Jun 24, 2021

Friendly bump, this PR is still active I'm just waiting for a review 🙂

@mx-psi
Copy link
Member

mx-psi commented Jun 24, 2021

@kvrhdn can you fix the merge conflict?

@jpkrohling can you have another look?

@mx-psi mx-psi requested a review from jpkrohling June 24, 2021 08:57
# Conflicts:
#	CHANGELOG.md
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.

Looks good, the changelog needs to be changed though.

@@ -4,7 +4,9 @@

## 💡 Enhancements 💡

- `tailsampling` processor: Add new policy `latency` (#3750)
- `tailsampling` processor:
- Add new policy `latency` (#3750)
Copy link
Member

Choose a reason for hiding this comment

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

I guess the latency made it to 0.29.0, no?

Copy link
Member

@jpkrohling jpkrohling Jun 24, 2021

Choose a reason for hiding this comment

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

Actually... 0.29.0 hasn't been released for contrib yet. I heard yesterday that the release train has departed already, though (#3863).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it still possible to get this PR in 0.29 or is the release already cut?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how to avoid a merge conflict with the release PR, should I wait until #3863 is merged to rebase and update the changelog?

Copy link
Member

Choose a reason for hiding this comment

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

I think @bogdandrutu mentioned yesterday that they were having build problems, blocking the release. I don't expect any new features to be included for 0.29.0. Wait for the release, then rebase and update the changelog.

processor/tailsamplingprocessor/sampling/status_code.go Outdated Show resolved Hide resolved
Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de>
@bogdandrutu bogdandrutu merged commit a3d91af into open-telemetry:main Jun 24, 2021
@yvrhdn yvrhdn deleted the status_code branch June 24, 2021 14:45
yvrhdn pushed a commit to yvrhdn/opentelemetry-collector-contrib that referenced this pull request Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants