Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

set CurrentRootSpan correctly #532

Merged

Conversation

mayurkale22
Copy link
Member

While debugging #526, I found we are not assigning currentRootSpan correctly, especially in case of tracer with cls. Ideally, we should set currentRootSpan before the onStartSpan event. This change is done in 92edaa9#diff-b46c2c01fbf239aae015e19185803e0dL154

The onStartSpan and onEndSpan events contains following check ->

if (this.currentRootSpan.traceId !== root.traceId) {
      this.logger.debug(
          'currentRootSpan != root on notifyStart. Need more investigation.');
    }

@mayurkale22
Copy link
Member Author

@hekike This is related to #484, Could you please review?

@hekike
Copy link
Contributor

hekike commented May 16, 2019

LGTM

Related topic: should a child span become a parent for the next child span with CLS?
It's not really possible with the current sync startChildSpan interface as we need to update span in CLS context.

@codecov-io
Copy link

Codecov Report

Merging #532 into master will decrease coverage by 0.25%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #532      +/-   ##
==========================================
- Coverage   95.21%   94.96%   -0.26%     
==========================================
  Files         147      147              
  Lines        9870     9568     -302     
  Branches      539      539              
==========================================
- Hits         9398     9086     -312     
- Misses        472      482      +10
Impacted Files Coverage Δ
src/common-utils.ts 75% <0%> (-20.24%) ⬇️
test/test-instana.ts 88.46% <0%> (-11.54%) ⬇️
test/test-stackdriver-cloudtrace.ts 92% <0%> (-6.96%) ⬇️
test/test-stackdriver-monitoring-utils.ts 93.33% <0%> (-6.67%) ⬇️
test/test-zipkin.ts 91.66% <0%> (-6.07%) ⬇️
src/trace/model/tracer.ts 66.66% <0%> (-3.11%) ⬇️
src/trace/model/tracer-base.ts 92.7% <0%> (-0.08%) ⬇️
src/trace/model/types.ts 100% <0%> (ø) ⬆️
src/trace/model/span.ts 97.6% <0%> (+0.01%) ⬆️

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 e5ff47c...471538c. Read the comment docs.

@mayurkale22
Copy link
Member Author

Related topic: should a child span become a parent for the next child span with CLS? It's not really possible with the current sync startChildSpan interface as we need to update span in CLS context.

Yes, currently it's not possible with startChildSpan interface. We already have issue for same #256. If you are feeling motivated, maybe you can attempt to write solution and open a PR. :)

@mayurkale22 mayurkale22 merged commit c30010d into census-instrumentation:master May 17, 2019
@mayurkale22 mayurkale22 deleted the setCurrentRootSpan branch May 17, 2019 23:19
@hekike
Copy link
Contributor

hekike commented May 18, 2019

@mayurkale22 what do you think about this proposal #498 it would make it possible.

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

Successfully merging this pull request may close these issues.

5 participants