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

Remove ParentSpanID from domain model #831

Merged
merged 2 commits into from May 18, 2018

Conversation

Projects
None yet
3 participants
@yurishkuro
Copy link
Member

commented May 18, 2018

Resolves #824

This implements option 1 from #824 by removing ParentSpanID from the domain model and only using the References array. ParentSpanID is replaced with an accessor ParentSpanID() that returns the span ID of the first child-of reference for the same trace ID.

Once unfortunate discovery: both Cassandra and Elasticsearch models kept the ParentID field in their respective models. It is no longer populated in the domain->db model conversions, but still has to be taken into account in the dbmodel->domain conversion for backwards compatibility with existing data. I don't know of a better solution.

A side effect of that is that we won't be able to use gogoprotobuf-generated model (from #773) to generate the JSON for Elasticsearch. The plan would be to move model/converter/json code into plugin/storage/es/dbmodel, similar to plugin/storage/cassandra/dbmodel, while the UI JSON model will be produced directly from protobuf model.


This change is Reviewable

@ghost ghost assigned yurishkuro May 18, 2018

@ghost ghost added the review label May 18, 2018

@coveralls

This comment has been minimized.

Copy link

commented May 18, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 3ed6401 on remove-parent-span-id into 38cee1b on master.

func (s *Span) ParentSpanID() SpanID {
for i := range s.References {
ref := &s.References[i]
if ref.TraceID == s.TraceID && ref.RefType == ChildOf {

This comment has been minimized.

Copy link
@black-adder

black-adder May 18, 2018

Collaborator

i've always wondered, why do we need traceid to be part of spanref? what use case is there when a span has a spanref to a different traceid?

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro May 18, 2018

Author Member

meta-traces; multi-trace joins in buffered writes

// ParentSpanID returns ID of a parent span if it exists.
// It searches for the first child-of reference pointing to the same trace ID.
func (s *Span) ParentSpanID() SpanID {
for i := range s.References {

This comment has been minimized.

Copy link
@black-adder

black-adder May 18, 2018

Collaborator

why not iterate over the reference directly? ie for _, ref :=

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro May 18, 2018

Author Member

each iteration will have to copy 4 words into the on-stack var

@yurishkuro yurishkuro force-pushed the remove-parent-span-id branch 3 times, most recently from 8b08701 to beb4a57 May 18, 2018

yurishkuro added some commits May 18, 2018

Remove ParentSpanID from domain model
Signed-off-by: Yuri Shkuro <ys@uber.com>
Add missing tests
Signed-off-by: Yuri Shkuro <ys@uber.com>

@yurishkuro yurishkuro force-pushed the remove-parent-span-id branch from beb4a57 to 3ed6401 May 18, 2018

@yurishkuro yurishkuro merged commit a00f2d9 into master May 18, 2018

5 of 6 checks passed

License Compliance FOSSA analyzing commit
Details
DCO All commits have a DCO sign-off from the author
Details
WIP ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@ghost ghost removed the review label May 18, 2018

mabn added a commit to mabn/jaeger that referenced this pull request May 28, 2018

Merge branch 'master' into influx
* master: (38 commits)
  Preparing release 1.5.0 (jaegertracing#847)
  Add bounds to memory storage (jaegertracing#845)
  Add metric for debug traces (jaegertracing#796)
  Change metrics naming scheme (jaegertracing#776)
  Bump gocql version (jaegertracing#829)
  Remove ParentSpanID from domain model (jaegertracing#831)
  Make gas run quiet (jaegertracing#838)
  Revert "Make gas run quite"
  Revert "Install gas from install-ci"
  Install gas from install-ci
  Make gas run quite
  Add 'gas' for security problems scanning (jaegertracing#830)
  Add ability to adjust static sampling probabilities per operation (jaegertracing#827)
  Support log-level flag on agent (jaegertracing#828)
  Remove unused function (jaegertracing#822)
  Add healthcheck to standalone (jaegertracing#784)
  Do not use KeyValue fields directly and use KeyValues as decorator only (jaegertracing#810)
  Add ContaAzul to the adopters list (jaegertracing#806)
  Add ISSUE_TEMPLATE and PULL_REQUEST_TEMPLATE (jaegertracing#805)
  Upgrade to  go 1.10 (jaegertracing#792)
  ...

# Conflicts:
#	cmd/agent/app/builder.go
#	cmd/collector/main.go
#	cmd/query/main.go
#	cmd/standalone/main.go

@pavolloffay pavolloffay deleted the remove-parent-span-id branch Nov 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.