Skip to content
This repository has been archived by the owner on May 23, 2024. It is now read-only.

All spans of a trace share sampling state #443

Merged
merged 8 commits into from
Oct 11, 2019

Conversation

vprithvi
Copy link
Contributor

@vprithvi vprithvi commented Oct 10, 2019

Initial stab at sharing span state. Part of #449.

Signed-off-by: Prithvi Raj p.r@uber.com

Which problem is this PR solving?

  • This allows sampling decisions to be updated

Short description of the changes

  • All spans for an individual trace share the sampling state

Signed-off-by: Prithvi Raj <p.r@uber.com>
@vprithvi vprithvi requested a review from yurishkuro October 10, 2019 18:31
@vprithvi vprithvi changed the title [WIP] Share span state [WIP] All spans of a trace share sampling state Oct 10, 2019
@codecov
Copy link

codecov bot commented Oct 10, 2019

Codecov Report

Merging #443 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #443      +/-   ##
==========================================
+ Coverage   84.68%   84.69%   +0.01%     
==========================================
  Files          33       33              
  Lines        1502     1503       +1     
==========================================
+ Hits         1272     1273       +1     
  Misses        168      168              
  Partials       62       62
Impacted Files Coverage Δ
log/zap/field.go 64.7% <0%> (+2.2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7048908...df4bee4. Read the comment docs.

context.go Outdated Show resolved Hide resolved
context.go Outdated Show resolved Hide resolved
context.go Outdated Show resolved Hide resolved
span.go Outdated Show resolved Hide resolved
span.go Outdated Show resolved Hide resolved
tracer.go Outdated Show resolved Hide resolved
tracer.go Outdated Show resolved Hide resolved
tracer_test.go Outdated Show resolved Hide resolved
transport_udp_test.go Outdated Show resolved Hide resolved
Signed-off-by: Prithvi Raj <p.r@uber.com>
context.go Outdated
s.setFlag(flagFirehose)
}

func (s *samplingState) resetFlags() {

Choose a reason for hiding this comment

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

func (*samplingState).resetFlags is unused (from unused)

Signed-off-by: Prithvi Raj <p.r@uber.com>
Signed-off-by: Prithvi Raj <p.r@uber.com>
context.go Outdated
flagSampled = byte(1)
flagDebug = byte(2)
flagFirehose = byte(8)
flagUnsampled = 0

Choose a reason for hiding this comment

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

flagUnsampled is unused (from varcheck)

Signed-off-by: Prithvi Raj <p.r@uber.com>
Signed-off-by: Prithvi Raj <p.r@uber.com>
context.go Outdated
flagSampled = byte(1)
flagDebug = byte(2)
flagFirehose = byte(8)
flagUnsampled = 0

Choose a reason for hiding this comment

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

flagUnsampled is unused (from deadcode)

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

lgtm. Looks like backwards compatible too.

context.go Outdated
}

type samplingState struct {
_flags atomic.Int32 // Only lower 8 bits are used. We use an int32 instead of a byte to use CAS operations
Copy link
Member

Choose a reason for hiding this comment

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

we never use underscores for field names, what's the reason to start now?

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 used it to avoid a conflict with flags(), but we can rename to ctxFlags or sth else

Copy link
Member

Choose a reason for hiding this comment

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

oic. Usually collisions are avoided by making methods public, like Flags.

Let's keep the underscore.

context.go Outdated Show resolved Hide resolved
transport_udp_test.go Outdated Show resolved Hide resolved
Signed-off-by: Prithvi Raj <p.r@uber.com>
Signed-off-by: Prithvi Raj <p.r@uber.com>
@vprithvi vprithvi changed the title [WIP] All spans of a trace share sampling state All spans of a trace share sampling state Oct 11, 2019
@yurishkuro yurishkuro merged commit ff64c7c into jaegertracing:master Oct 11, 2019
@funny-falcon-at-joomcode

I used per-span sampling decisions to dynamically skip unimportant sub-spans (ie too short to be interesting).
After this change I have:

  • some spans are already sent to agent, therefore they are visible, but has no root span.
  • spans after "non-important" ones are not visible at all...

"Backward compatibility" is great, yeah...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants