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

startChildSpan with SpanOptions interface only. #521

Merged
merged 2 commits into from
May 15, 2019

Conversation

mayurkale22
Copy link
Member

Continuation of #484, this PR is to cleanup and match internal interfaces with top level APIs. i.e. Internal startChildSpan interface accepts same argument(SpanOptions) as public facing Tracer's startChildSpan.

This PR does not contain any breaking change.

@mayurkale22
Copy link
Member Author

/cc @hekike FYI.

@codecov-io
Copy link

codecov-io commented May 14, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #521      +/-   ##
==========================================
- Coverage   95.27%   94.75%   -0.52%     
==========================================
  Files         147      147              
  Lines       10808    10491     -317     
  Branches      907      911       +4     
==========================================
- Hits        10297     9941     -356     
- Misses        511      550      +39
Impacted Files Coverage Δ
src/detect-resource.ts 60% <0%> (-30.91%) ⬇️
test/test-http2.ts 77.83% <0%> (-18.99%) ⬇️
src/common-utils.ts 95.83% <0%> (-4.17%) ⬇️
src/tags/propagation/variant-encoding.ts 96% <0%> (-4%) ⬇️
test/test-instana.ts 85.18% <0%> (-3.71%) ⬇️
test/test-jaeger-format.ts 95.38% <0%> (-1.8%) ⬇️
test/test-detect-resource.ts 97.43% <0%> (-1.71%) ⬇️
src/tags/propagation/binary-serializer.ts 95.58% <0%> (-1.52%) ⬇️
src/trace/model/no-record/no-record-span.ts 57.53% <0%> (-0.91%) ⬇️
test/test-ioredis.ts 97.16% <0%> (-0.55%) ⬇️
... and 35 more

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 1a77de1...622b401. Read the comment docs.

@@ -184,25 +184,12 @@ export class NoRecordSpan implements types.Span {

/**
* Starts a new no record child span in the no record root span.
* @param nameOrOptions Span name string or SpanOptions object.
* @param kind Span kind if not using SpanOptions object.
* @param [options] A SpanOptions object to start a child span.
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the role of the brackets [ ] around options in the JSDoc? Does that signify that it's optional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, brackets are used to indicate optional param.

Copy link
Contributor

@hekike hekike left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning it up!

@hekike
Copy link
Contributor

hekike commented May 14, 2019

Should we add these to the changelog update?

@mayurkale22
Copy link
Member Author

Should we add these to the changelog update?

I thought about it, but as this is an internal interface change I didn't include in CHANGELOG. WDYT?

@hekike
Copy link
Contributor

hekike commented May 14, 2019

Hmm, technically you can require @opencensus/core which exports types: https://github.com/census-instrumentation/opencensus-node/blob/master/packages/opencensus-core/src/index.ts#L19 But I don't feel strong about it.

@mayurkale22
Copy link
Member Author

Yeah, this is a valid point. I will update the CHAGELOG. Thanks

@mayurkale22 mayurkale22 merged commit 800520f into census-instrumentation:master May 15, 2019
@mayurkale22 mayurkale22 deleted the startChildSpan branch May 15, 2019 17:13
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.

None yet

5 participants