Skip to content

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Oct 16, 2020

This adds a pointer to the Span class which points the the Transaction instance from which the span is descended. (It doesn't do anything with it yet, though - that will come in a subsequent PR which allows unsampled transactions living on the scope to be found.)

@rhcarvalho
Copy link
Contributor

For reference, the WIP Go implementation always records the span tree, no matter what the sampling decision is.
Would be curious if that has any negative implication, or otherwise we could do the same in Python and JS to eliminate having "yet-another-way" to store references to spans.

@lobsterkatie lobsterkatie force-pushed the kmclb-make-spans-point-to-their-transactions branch from 5a58206 to 85604e3 Compare October 19, 2020 16:42
@lobsterkatie
Copy link
Member Author

For reference, the WIP Go implementation always records the span tree, no matter what the sampling decision is.
Would be curious if that has any negative implication, or otherwise we could do the same in Python and JS to eliminate having "yet-another-way" to store references to spans.

This seems better to me for two reasons:

  1. Why take up memory storing a span tree if we're not going to keep its data? This does make span trees we keep a teeny bit heavier while we're building them, but on the balance it's still a memory savings unless the sample rate is super high.

  2. self._span_recorder[0] is a pretty darn opaque way to say "the transaction to which this span belongs," and feels more on the level of implementation detail than ideally even an internal API would be (IMHO).

@lobsterkatie lobsterkatie changed the title [WIP] feat(tracing): Make spans point to their transactions feat(tracing): Make spans point to their transactions Oct 19, 2020
@lobsterkatie lobsterkatie merged commit 5bb6ffc into master Oct 21, 2020
@lobsterkatie lobsterkatie deleted the kmclb-make-spans-point-to-their-transactions branch October 21, 2020 18:32
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.

3 participants