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

feat(suspect-spans): Add span exclusive time #1061

Merged
merged 12 commits into from
Sep 1, 2021

Conversation

Zylphrex
Copy link
Member

This change adds the computation for the span's exclusive time if it is enabled.

The span's exclusive time can be understood as the amount of time spent in this
span excluding the time spent in any immediate children.

This change adds the computation for the span's exclusive time if it is enabled.

The span's exclusive time can be understood as the amount of time spent in this
span excluding the time spent in any immediate children.
@Zylphrex Zylphrex marked this pull request as ready for review August 23, 2021 18:32
@Zylphrex Zylphrex requested review from a team August 23, 2021 18:32
@dashed
Copy link
Member

dashed commented Aug 23, 2021

@Zylphrex will you be adding an E2E integration test for this PR?

Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

This looks great overall, thanks for the thorough patch.
A few comments below.

relay-general/src/protocol/span.rs Show resolved Hide resolved
relay-general/src/store/mod.rs Show resolved Hide resolved
relay-general/src/store/normalize.rs Outdated Show resolved Hide resolved
relay-general/src/store/normalize/spans.rs Outdated Show resolved Hide resolved
relay-general/src/store/normalize/spans.rs Outdated Show resolved Hide resolved
relay-general/src/store/normalize/spans.rs Outdated Show resolved Hide resolved
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for addressing the comments. Good to merge once TimeInterval is deduplicated.

Please add a changelog entry in the "Internal" section.

relay-general/src/store/normalize/breakdowns.rs Outdated Show resolved Hide resolved
relay-general/src/store/normalize/spans.rs Outdated Show resolved Hide resolved
@jan-auer
Copy link
Member

jan-auer commented Sep 1, 2021

Good to merge, please view both of my comments as optional.

Copy link
Member

@k-fish k-fish left a comment

Choose a reason for hiding this comment

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

👍

@Zylphrex Zylphrex merged commit bc6914f into master Sep 1, 2021
@Zylphrex Zylphrex deleted the feat/add-span-exclusive-time branch September 1, 2021 17:14
Zylphrex added a commit that referenced this pull request Sep 10, 2021
Following up with #1061, this change adds the exclusive time calculation for the
root span of a transaction.
Zylphrex added a commit that referenced this pull request Sep 22, 2021
Following up with #1061, this change adds the exclusive time calculation for the
root span of a transaction.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants