Add trace params by vigneshtdev · Pull Request #311 · census-instrumentation/opencensus-node

vigneshtdev · 2019-01-28T19:24:08Z

Addresses #285.
Unit Tests included as well.

Update to main branch

merging all ga effort commits

codecov-io · 2019-01-28T19:27:35Z

Codecov Report

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

@@            Coverage Diff             @@
##           master     #311      +/-   ##
==========================================
+ Coverage   94.75%   94.79%   +0.04%     
==========================================
  Files         114      114              
  Lines        7603     7663      +60     
  Branches      692      701       +9     
==========================================
+ Hits         7204     7264      +60     
  Misses        399      399

Impacted Files	Coverage Δ
src/trace/sampler/sampler.ts	`100% <0%> (ø)`	⬆️
test/test-sampler.ts	`100% <0%> (ø)`	⬆️
test/test-tracer.ts	`100% <0%> (ø)`	⬆️
src/trace/model/tracer.ts	`85.58% <0%> (+1.11%)`	⬆️

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 de34faf...2dcab54. Read the comment docs.

draffensperger · 2019-01-29T00:45:06Z

packages/opencensus-core/src/trace/config/types.ts

  logger?: Logger;
 }

+/** Specify the deafault Tracer Paramaters */


Can this commented code be removed?

I saw this was marked as resolved - is the commented out code below intentional? (Or maybe just a commit not pushed yet?)

packages/opencensus-core/src/trace/config/types.ts

draffensperger · 2019-01-29T00:47:08Z

packages/opencensus-core/src/trace/config/types.ts

+
+// }
+
+export interface TraceParameters {


Could you add a top level comment for this interface as a whole?

draffensperger · 2019-01-29T00:47:58Z

packages/opencensus-core/src/trace/model/tracer.ts

  logger: loggerTypes.Logger = logger.logger();
-
+  /** maxAnnotationEvents is max number of annotation events per span  */
+  numberOfAnnontationEventsPerSpan?: number;


Rather than storing the individual values here, could it just store something like traceParameters: TraceParameters = DEFAULT_TRACE_PARAMETERS?

Then if we add one more parameter later on, we don't need to add it two places.

I have a question, do we assume that user won't configure the tracer with parameters having values more than their limit?

Ah, I missed the logic of the builder pattern below that capped those values at their limits. If other languages have that cap, we should preserve it for Node I'd think.

In that case, I would recommend making these be top-level attributes (like you have) but be made readonly to prevent changes to them once they are initialized.

packages/opencensus-core/src/trace/model/tracer.ts

draffensperger · 2019-01-29T00:52:19Z

packages/opencensus-core/src/trace/model/tracer.ts

    this.logger = this.config.logger || logger.logger();
    this.sampler = SamplerBuilder.getSampler(config.samplingRate);
+    if (config.traceParameters != null) {
+      this.numberOfAnnontationEventsPerSpan =


What would you think about integrating the default values right in line here rather than abstracting it into a builder class?

It could just be something like:

this.numberOfAnnontationEventsPerSpan = Math.min(MAX_NUMBER_OF_ANNOTATION_EVENTS_PER_SPAN, config.traceParameters.numberOfAnnontationEventsPerSpan || MAX_NUMBER_OF_ANNOTATION_EVENTS_PER_SPAN)

and similar for the other parameters.

I personally think the builder pattern, while it makes sense for other languages is not needed in TypeScript.

I agree with @draffensperger, builder pattern is not required here.

#311 (comment)

There is actually no builder pattern implemented here, but it does have the methods for creating traceParameters so I used the builder suffix for the method. Should I rename the method to something else?

draffensperger · 2019-01-29T00:53:44Z

Thanks for the contribution, great to have this moving ahead!! I left a number of nit-picky comments - feel free to explain if you disagree on any of these.

mayurkale22

Could you please add a line in CHANGELOG.md

packages/opencensus-core/src/trace/config/types.ts

packages/opencensus-core/src/trace/sampler/sampler.ts

packages/opencensus-core/test/test-sampler.ts

rghetia · 2019-01-28T22:50:45Z

packages/opencensus-core/src/trace/config/types.ts

+   * span
+   */
+  numberOfAnnontationEventsPerSpan?: number;
+  /** numberOfMessageEventsPerSpan is number of message events per span */


nit: s/is number/is max number/

Same for attributes and links

in the interface definition, it should just be number of right?

it is a maximum number allowed. So it is a max value.

Fix Typos.

draffensperger · 2019-01-30T17:57:55Z

packages/opencensus-core/src/trace/model/tracer.ts

  /** Bit to represent whether trace is sampled or not. */
  private readonly IS_SAMPLED = 0x1;
+  /** Default Limit for Annotations per span */
+  private readonly DEFAULT_SPAN_MAX_NUM_ANNOTATIONS = 32;


Could you make these be top level constants rather than properties of the class (that would be set on each instance)?

packages/opencensus-core/src/trace/sampler/sampler.ts

packages/opencensus-core/test/test-sampler.ts

draffensperger

LGTM pending final comments. Thanks for your work on this!

packages/opencensus-core/src/trace/model/tracer.ts

…ncensus-node into add-trace-params

mayurkale22 · 2019-01-30T23:49:57Z

@vigneshtdev is this ready to merge?

vigneshtdev · 2019-01-31T17:14:29Z

@vigneshtdev is this ready to merge?

Yeah, you can go ahead and merge it. I don't have merging rights

vigneshtdev and others added 5 commits January 17, 2019 20:25

Merge pull request #1 from census-instrumentation/master

6445f2e

Update to main branch

Merge pull request #3 from census-instrumentation/master

1b284c7

merging all ga effort commits

Add Trace Parameters limits

e7bc00b

Merge branch 'master' of https://github.com/vigneshtdev/opencensus-node

0d98f7b

Merge branch 'master' of https://github.com/census-instrumentation/op…

79f969a

…encensus-node into add-trace-params

googlebot added the cla: yes label Jan 28, 2019

mayurkale22 requested review from draffensperger, mayurkale22 and rghetia January 28, 2019 22:28

draffensperger reviewed Jan 29, 2019

View reviewed changes

mayurkale22 reviewed Jan 29, 2019

View reviewed changes

rghetia reviewed Jan 29, 2019

View reviewed changes

Improve null checking, naming conventions..Address CR comments.

d119467

vigneshtdev mentioned this pull request Jan 30, 2019

Add Trace Params #311 #315

Closed

vigneshtdev added 2 commits January 30, 2019 23:19

Fix Typos.

6542bba

Improve null checking, naming conventions..Address CR comments.

7777646

Fix Typos.

draffensperger reviewed Jan 30, 2019

View reviewed changes

Import constants from TraceParamsBuilder

8d9caa9

draffensperger reviewed Jan 30, 2019

View reviewed changes

packages/opencensus-core/test/test-sampler.ts Show resolved Hide resolved

draffensperger approved these changes Jan 30, 2019

View reviewed changes

mayurkale22 mentioned this pull request Jan 30, 2019

Enforce TraceParams Limits #316

Closed

mayurkale22 reviewed Jan 30, 2019

View reviewed changes

packages/opencensus-core/src/trace/model/tracer.ts Outdated Show resolved Hide resolved

vigneshtdev added 2 commits January 31, 2019 01:01

Add unit test for verifying max parameter values.Address CR comments

3d8e965

Merge branch 'add-trace-params' of https://github.com/vigneshtdev/ope…

2dcab54

…ncensus-node into add-trace-params

mayurkale22 approved these changes Jan 30, 2019

View reviewed changes

mayurkale22 merged commit f98ec08 into census-instrumentation:master Jan 31, 2019

vigneshtdev deleted the add-trace-params branch January 31, 2019 18:13

mayurkale22 mentioned this pull request Jan 31, 2019

Add TraceParams Limits #285

Closed

mayurkale22 mentioned this pull request Feb 12, 2019

chore(multiple): 0.0.9 release proposal #341

Merged

Conversation

vigneshtdev commented Jan 28, 2019

Uh oh!

codecov-io commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vigneshtdev Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

draffensperger Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

draffensperger commented Jan 29, 2019

Uh oh!

mayurkale22 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

draffensperger Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

draffensperger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mayurkale22 commented Jan 30, 2019

Uh oh!

vigneshtdev commented Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Jan 28, 2019 •

edited

Loading

vigneshtdev Jan 30, 2019 •

edited

Loading

draffensperger Jan 29, 2019 •

edited

Loading

draffensperger Jan 30, 2019 •

edited

Loading

vigneshtdev commented Jan 31, 2019 •

edited

Loading