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: implement child span count #674

Open
wants to merge 1 commit into
base: master
from

Conversation

@mayurkale22
Copy link
Member

mayurkale22 commented Jan 7, 2020

Which problem is this PR solving?

  • Implement child span count required by the Collector and Stackdriver exporter.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 7, 2020

Codecov Report

Merging #674 into master will decrease coverage by 1.72%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #674      +/-   ##
==========================================
- Coverage   91.56%   89.84%   -1.73%     
==========================================
  Files         217      212       -5     
  Lines       10186    10203      +17     
  Branches      911      938      +27     
==========================================
- Hits         9327     9167     -160     
- Misses        859     1036     +177
Impacted Files Coverage Δ
.../opentelemetry-exporter-collector/src/transform.ts 96.96% <ø> (ø) ⬆️
.../opentelemetry-exporter-jaeger/test/jaeger.test.ts 100% <ø> (ø) ⬆️
...es/opentelemetry-exporter-collector/test/helper.ts 100% <ø> (ø) ⬆️
...ages/opentelemetry-exporter-collector/src/types.ts 100% <ø> (ø) ⬆️
.../opentelemetry-exporter-zipkin/test/zipkin.test.ts 100% <ø> (ø) ⬆️
...entelemetry-exporter-jaeger/test/transform.test.ts 100% <100%> (ø) ⬆️
packages/opentelemetry-tracing/src/BasicTracer.ts 100% <100%> (ø) ⬆️
packages/opentelemetry-tracing/src/Span.ts 100% <100%> (ø) ⬆️
...ges/opentelemetry-tracing/test/BasicTracer.test.ts 100% <100%> (ø) ⬆️
packages/opentelemetry-core/test/utils/url.test.ts 50% <0%> (-50%) ⬇️
... and 42 more
Copy link
Member

vmarchaud left a comment

lgtm, but i'm not sure to understand if it's per spec or just because we need it in the stackdriver exporter ?

@mayurkale22

This comment has been minimized.

Copy link
Member Author

mayurkale22 commented Jan 8, 2020

lgtm, but i'm not sure to understand if it's per spec or just because we need it in the stackdriver exporter ?

Thanks for raising this. For now, I have added Hold and waiting-for-spec labels. Will get back to this later.

Edit: This is also used by the Collector in Jaeger translator: https://github.com/open-telemetry/opentelemetry-collector/blob/dc6b290e3cfb0405d7df4ded435ec69e4b7594be/translator/trace/jaeger/protospan_to_jaegerproto.go#L529

@OlivierAlbertini

This comment has been minimized.

Copy link
Member

OlivierAlbertini commented Jan 8, 2020

lgtm, but i'm not sure to understand if it's per spec or just because we need it in the stackdriver exporter ?

Thanks for raising this. For now, I have added Hold and waiting-for-spec labels. Will get back to this later.

Edit: This is also used by the Collector in Jaeger translator: https://github.com/open-telemetry/opentelemetry-collector/blob/dc6b290e3cfb0405d7df4ded435ec69e4b7594be/translator/trace/jaeger/protospan_to_jaegerproto.go#L529

By curiosity, What's the purpose of ChildSpanCount ? Is it for guessing what span is not ended so Do I have leak or not ?

@mayurkale22

This comment has been minimized.

Copy link
Member Author

mayurkale22 commented Jan 8, 2020

By curiosity, What's the purpose of ChildSpanCount ? Is it for guessing what span is not ended so Do I have leak or not ?

AFAIK If set, allows implementations to detect missing child spans. In case of Jaeger, the Collector is adding as a part of the tags.

@vmarchaud

This comment has been minimized.

Copy link
Member

vmarchaud commented Jan 9, 2020

@mayurkale22 I believe it would be better to merge this with the ChildSpanCount as a Tag, then when the spec is updated implement it in the exported Span, WDYT ?

@mayurkale22 mayurkale22 removed this from the Alpha v0.3.3 milestone Jan 15, 2020
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.