refactor: address opencensus-core TODOs by eduardoemery · Pull Request #47 · census-instrumentation/opencensus-node

eduardoemery · 2018-06-15T11:38:33Z

No description provided.

googlebot · 2018-06-15T11:38:35Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

kjin · 2018-06-15T23:23:51Z

  level: string;

-
-  // TODO: reevaluate options to accept numbers as a parameter


For each TODO that is removed without changes, please document (as a PR comment) why you decided not to do it.

Ok, I'll comment those. This one in specific was removed because being able to use numbers as parameters gives more freedom to the end user. Also, passing number as parameters is being currently used across the implementation.

Makes sense, thanks.

kjin · 2018-06-15T23:26:48Z

   */
-  publish(rootSpans: modelTypes.RootSpan[]): void;
+  // tslint:disable-next-line:no-any
+  publish(rootSpans: modelTypes.RootSpan[]): Promise<any>|void;


Two notes:

What is the meaning of the deferred value being returned here? How are users expected to treat this value? If there are no good answers to this question it should just be Promise<void>.

Returning either a Promise or void complicates handling the return value. Can we only return Promise?

The ideia is that by returning Promise<any> | void we account for publish methods that returns a Promise, while giving freedom to future collaborators to make a method that returns void. Currently, NoopExporter and ConsoleExporter return void, but if you prefer, I can change them to return Promise<void> and restrict the interface, no problems with that IMO.

It's OK for publish to inherently not contain anything asynchronous, but it would still be more consistent to return a Promise.resolve() instead of void anyway. If we consider the code that will call this function:

const result = exporter.publish(); if (result) { result.then(next); } else { next(); // note: zalgo alert }

Why even have this branching logic, when we could just do

exporter.publish().then(next);

That was the original context behind the second question. If you could make the change in *Exporter that would be great.

Hmm, got it. Made the change but had to make Promise<number|string|void> to account for some exporter implementations that resolve number or string.

Promise<number|string|void> goes back to my original first question. Do we use the return value in a meaningful way? If so, it would be better to standardize now what that return value is (maybe a boolean or status code). Otherwise, if we don't use it, we should just make it void.

BTW, the answer to this question might not exist today. It's hard to know how to standardize the return value if we are currently not using it. So I can merge this PR, just something to think about.

I could see the return value being used in tests only, but not in applications. I'm not sure if we should standardize it tho, goes back to that thing about future contributors. But I understand your point of view. Would be great to discuss that latter.

kjin · 2018-06-15T23:29:24Z

+   */
+  abstract applyPatch(
+      // tslint:disable:no-any
+      moduleExports: any, tracer: modelTypes.Tracer, version: string): any;


While we are here... maybe it makes sense to parameterize the return value. We know the moduleExports and the return value should be the same type, so we can make it applyPatch<T>(moduleExports: T, ...): T;.

Makes a lot of sense. As oc-core changed since this PR was opened, I made this change to the enable method.

kjin · 2018-06-15T23:30:38Z

    "ts-node": "^4.0.0",
-    "typescript": "~2.6.1",
-    "nyc": "11.6.0"
+    "intercept-stdout": "^0.1.2",


Can you preserve the alphabetical ordering here?

eduardoemery · 2018-06-20T12:04:44Z

 }

-
-// TODO: reevaluate the need to create a new logger instance on every call to


By creating a new logger instance on every call to it we allow distinct classes to have distinct log levels.

I think the original comment that this was in reference to is that every time logger was called, the field logger['logger'] was being replaced. This field is also never getting read. The logger method is otherwise just a call-through to new ConsoleLogger.

I would prefer that logger not even exist in the first place, and that anywhere it's used, new ConsoleLogger is used instead. After all, why do we need the wrapper method?

See: #2 (comment)

My mistake, I got the TODO confused. Regarding this one, our idea here is to return an instance of logger, although calling new ConsoleLogger() works too. We drove inspiration from this logDriver.

I removed the logger['logger'] = aLogger; that had no need to be there at all.

eduardoemery · 2018-06-20T12:07:15Z

 import * as crypto from 'crypto';

-
-// TODO: rethink this snippet aproach


There was no need for change here.

kjin

Left some replies, what do you think?

eduardoemery · 2018-06-21T16:19:33Z

Sorry for the delay. Took a look at your comments, thanks for reviewing. Made a few changes.

kjin · 2018-06-21T17:17:45Z

@eduardoemery Maybe you're in the middle of it right now, but currently the review comment changes only include package-lock.json. Did you miss adding a file to the commit?

eduardoemery · 2018-06-22T11:04:25Z

Yesterday morning I had some trouble with my git. Got it fixed but apparently it impacted here. Fixed now, sorry for the trouble caused.

kjin · 2018-06-22T18:36:29Z

-  const aLogger = new ConsoleLogger(options);
-  logger['logger'] = aLogger;
-  return aLogger;
+  return new ConsoleLogger(options);


Nit: Make this function have return type ConsoleLogger

Sure. I used the interface types.Logger instead of ConsoleLogger to keep the pattern of using interfaces. But let me know if you don't agree.

types.Logger is better. Thanks!

…ocal-storage

googlebot · 2018-06-22T21:10:00Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

* refactor: changes to address opencensus-core TODOs * refactor: reveiw TODO for clearCurrentTrace when using continuation-local-storage * refactor(fix): changes to address review comments Co-authored-by: Fabio Silva <fgs@cesar.org.br>

googlebot added the cla: no label Jun 15, 2018

kjin reviewed Jun 15, 2018

View reviewed changes

eduardoemery commented Jun 20, 2018

View reviewed changes

eduardoemery force-pushed the refactor_address_core_TODOs branch from 5a6efe5 to 1a39e37 Compare June 20, 2018 12:06

eduardoemery commented Jun 20, 2018

View reviewed changes

kjin reviewed Jun 20, 2018

View reviewed changes

eduardoemery force-pushed the refactor_address_core_TODOs branch 3 times, most recently from 394a12d to f807861 Compare June 21, 2018 16:46

eduardoemery force-pushed the refactor_address_core_TODOs branch 2 times, most recently from d94f3ad to e044058 Compare June 22, 2018 10:57

eduardoemery force-pushed the refactor_address_core_TODOs branch from e044058 to b64198a Compare June 22, 2018 12:24

kjin reviewed Jun 22, 2018

View reviewed changes

eduardoemery and others added 2 commits June 22, 2018 16:19

refactor: changes to address opencensus-core TODOs

7c8ab6f

refactor: reveiw TODO for clearCurrentTrace when using continuation-l…

6684710

…ocal-storage

eduardoemery force-pushed the refactor_address_core_TODOs branch from b64198a to c580bfb Compare June 22, 2018 19:21

refactor(fix): changes to address review comments

9c2523d

eduardoemery force-pushed the refactor_address_core_TODOs branch from c580bfb to 9c2523d Compare June 22, 2018 19:29

kjin added the cla: yes label Jun 22, 2018

kjin removed the cla: no label Jun 22, 2018

kjin approved these changes Jun 22, 2018

View reviewed changes

kjin merged commit db46f4f into census-instrumentation:master Jun 22, 2018

		level: string;


		// TODO: reevaluate options to accept numbers as a parameter

		}


		// TODO: reevaluate the need to create a new logger instance on every call to

		import * as crypto from 'crypto';


		// TODO: rethink this snippet aproach

Conversation

eduardoemery commented Jun 15, 2018

Uh oh!

googlebot commented Jun 15, 2018

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!

Choose a reason for hiding this comment

Uh oh!

kjin Jun 20, 2018 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

kjin Jun 20, 2018 • 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!

kjin left a comment

Choose a reason for hiding this comment

Uh oh!

eduardoemery commented Jun 21, 2018

Uh oh!

kjin commented Jun 21, 2018

Uh oh!

eduardoemery commented Jun 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eduardoemery Jun 22, 2018 • 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!

googlebot commented Jun 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kjin Jun 20, 2018 •

edited

Loading

kjin Jun 20, 2018 •

edited

Loading

eduardoemery Jun 22, 2018 •

edited

Loading