refactor: index.ts to async-first

[AVaksman]

Fixes #390

1. Refactor to async-first with callbackifyAll
   • Currently logging.request uses callback to differentiate between making callbackRequest and
     streamRequest'. But now presence of callback will be a flag for callbackify`

As such

2. Refactor methods to call gax client methods directly.
   • To solve above mentioned issue.
   • For readability.

[codecov]

Codecov Report

Merging #412 into master will increase coverage by 0.42%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##           master     #412      +/-   ##
==========================================
+ Coverage   91.33%   91.76%   +0.42%     
==========================================
  Files          14       14              
  Lines         681      692      +11     
  Branches       34       38       +4     
==========================================
+ Hits          622      635      +13     
+ Misses         41       39       -2     
  Partials       18       18

Impacted Files	Coverage Δ
src/index.ts	99.47% <100%> (+0.02%)	⬆️
src/log.ts	81.6% <86.36%> (+0.43%)	⬆️
src/v2/config_service_v2_client.js	88.7% <0%> (+0.8%)	⬆️
src/v2/logging_service_v2_client.js	82.05% <0%> (+1.28%)	⬆️

---

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 c6265e3...7771526. Read the comment docs.

[stephenplusplus]

It's awesome seeing this pattern, where we can get the project ID in an un-obtrusive way with await 👍 I think we have a chance to ditch the {{projectId}} placeholder token here altogether. If we just get the project ID before we build the reqOpts object (L#360), then we know this.projectId is the right value-- no need to use the token for later replacement, since if we can't get it now, we can't get it later, either.

[stephenplusplus]

Tagging related issue which seeks to destroy the placeholder: googleapis/nodejs-common#10

[AVaksman]

This method is dependent on objects from other libraries storage.bucket, bigQuery.dataset or pubsub.topic which could still have {{projectId}} placeholder. I would have to still run replaceProjectIdToken after building the reqOpts object. So I placed it after, to have it in one place for both.

[stephenplusplus]

Good catch, that is reasonable. I believe this specific method is unique, so I might have picked a bad one to make this suggestion on. For the others, could we skip the setProjectId() call, and instead just put the real projectId right where we need it in the reqOpts?

[AVaksman]

@stephenplusplus
It looks like I can skip setProjectId() getSinks and getSinksStream since these methods do not depend on the Sink constructor.

I am leaving getEntries and getEntriesStream as is since these guys do depend on the Log constructor and I would still need to to replaceProjectIdToken for options.filter

nodejs-logging/src/log.ts

Lines 525 to 530 in 81138d6

	options = extend(
	{
	filter: 'logName="' + this.formattedName_ + '"',
	},
	options);
	return this.logging.getEntries(options as GetEntriesRequest, callback!);

from

nodejs-logging/src/log.ts

Lines 108 to 110 in 81138d6

	constructor(logging: Logging, name: string, options?: LogOptions) {
	options = options || {};
	this.formattedName_ = Log.formatName_(logging.projectId, name);

[stephenplusplus]

For getEntries/getEntriesStream, I think we can do the refresh right there:

this.projectId = await this.auth.getProjectId()
this.formattedName_ = Log.formatName(this.projectId, this.name)
options = extend({ filter: `logName="${this.formattedName_}"` })

// for getEntriesStream
this.auth.getProjectId().then(projectId => {
  this.projectId = projectId
  this.formattedName_ = Log.formatName(this.projectId, this.name)
  options = extend({ filter: `logName="${this.formattedName_}"` })
})

The mission for a while has been to eliminate that token altogether, but it was the absence of async/await that has prevented it. But now that we have it, anywhere this.formattedName_ is used, it should be replaced with code similar to the above, in this file and others where possible. Just a thought, but maybe formatName() should be repurposed to do the project ID fetching as well.

[JustinBeckwith]

@AVaksman apologies for the merge conflicts on this one...

[JustinBeckwith]

Stepping back so @stephenplusplus can do the review on this one :)

[JustinBeckwith]

@AVaksman @stephenplusplus can we pick this back up? The PR has been sitting around for a while.

[AVaksman]

@stephenplusplus PTAL.
In log.getEntriesStream() I am setting a "flag" and moving setting the logName filter to main logging.getEntriesStream() method where I already have the proper projectId.

[stephenplusplus]

Thanks! I'm okay with that given the tradeoff for simplicity. Instead of making it a pseudo-private option, though, let's just own it and make it a feature of the method and call it log. We would accept just the short name, and then we'll format it correctly per the formatName method.

[AVaksman]

Thanks! I'm okay with that given the tradeoff for simplicity. Instead of making it a pseudo-private option, though, let's just own it and make it a feature of the method and call it log. We would accept just the short name, and then we'll format it correctly per the formatName method.

Thanks. Done
Follow up question. Currently if user passes filter to log.getEntries/log.getEntriesStream or filter and log to logging.getEntries/logging.getEntriesStream the filter will prevail. Should it be documented or prevented via exception?

[AVaksman]

Follow up question. Currently if user passes filter to log.getEntries/log.getEntriesStream or filter and log to logging.getEntries/logging.getEntriesStream the filter will prevail. Should it be documented or prevented via exception?

Proposing to construct an advanced filter using AND in case both user-provided and logName filters are present.

[stephenplusplus]

I agree with that suggestion 👍

[AVaksman]

@stephenplusplus
I pushed my proposed code changes in 72fefc1
So this PR was intended to introduce async-first approach in index.ts only (as a start).
Would you like add changes across the whole repo here?

Is anything else needed for this PR?

P.S.
Do you know why install.ts of the system-test is failing?

[bcoe]

@AVaksman just doing some triage, it looks like system tests are failing with the following error:

DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Have you had a chance to look into this at all?

[AVaksman]

@bcoe
somewhere in const logging = new Logging(); a util.inspect() functions is called which throws a deprecation warning DEP0079 which fails the last test.

Investigating.

Update:
change console.log() to console.dir() to bypass any custom inspect() calls which are deprecated.

Note: this issue goes away with Node 11 obviously.

[stephenplusplus]

Would you like add changes across the whole repo here?
Is anything else needed for this PR?

I think if going file by file is easier, that's fine. Thanks for getting all of those changes in!