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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: netLog API for dynamic logging control #13068

Merged
merged 29 commits into from Jun 19, 2018

Conversation

Projects
None yet
5 participants
@sethlu
Member

sethlu commented May 24, 2018

馃毀 Rather than having to explicitly pass --log-net-log to the executable before Electron launches, we can call net.startLogging(filename) and net.stopLogging([callback]) to control when network logging begins and finishes. To conform better with the existing API, --log-net-log will still cause logging to start at launch , but we can programmatically stop observing with net.stopLogging([callback]) .

  • Tests
  • Docs
Introduce `net.{start|stop}Logging()`
- Slight regression right now as Electron won't automatically start logging net-logs at launch, will soon be fixed
- To implement callback for async controls

@sethlu sethlu requested a review from electron/reviewers as a code owner May 24, 2018

@sethlu sethlu self-assigned this May 24, 2018

sethlu added some commits May 25, 2018

Fix small regression on --log-net-log
--log-net-log should work again
@MarshallOfSound

Requesting changes because there isn't a "request answers" button 馃槃

Not sure if this 100% lives in the net module. I know it's called the "net log" but the net module is attempting API compatibility with the http and https modules of node.js just using chromiums network stack. I'm thinking these might be better suited in a new module called netLog

Thoughts?

@@ -22,12 +30,48 @@ v8::Local<v8::Value> Net::Create(v8::Isolate* isolate) {
return mate::CreateHandle(isolate, new Net(isolate)).ToV8();
}
void Net::StartLogging(mate::Arguments* args) {
if (net_log_) {

This comment has been minimized.

@MarshallOfSound

MarshallOfSound May 26, 2018

Member

Is there a case where net_log_ would not be here, and if so should we be throwing an error for that case (args->ThrowError). Not sure the case where net_log_ won't be a thing

This comment has been minimized.

@sethlu

sethlu May 26, 2018

Member

馃挴 I added the checks when I first thought the NetLog is optionally instantiated in Electron based on --log-net-log. Let me have them removed 馃樃

@sethlu

This comment has been minimized.

Member

sethlu commented May 26, 2018

@MarshallOfSound 馃憤 Right the term net is quite confusing between how Chromium and Node sees it. If we are trying to imitate the net module from Node, probably it's a good idea to separate the logging features from net. Whilst I feel netlog is not general enough to be a submodule by itself?

My initial thinking is that since our net module essentially exposes Chromium's net namespace so the logging falls in here. How does a net.debug.{start|stop}Logging() spec sound?

@sethlu

This comment has been minimized.

Member

sethlu commented May 27, 2018

@MarshallOfSound After the API definition is decided I'll update the docs.

it('should begin and stop logging to file when .startLogging() and .stopLogging() is called', (done) => {
net.startLogging(dumpFile)
net.stopLogging(() => {

This comment has been minimized.

@sindresorhus

sindresorhus May 28, 2018

Contributor

The plan is to Promisify all of Electron's APIs, so I don't think it makes sense to introduce new APIs with callback interface. This should return a promise that can be awaited.

This comment has been minimized.

@sethlu

sethlu May 28, 2018

Member

LOL yea the current native implementation does a callback since it's more straightforward to implement on the C++ end. Once we find a good place for these API methods to live I think I can expose .stopLogging() as an async method.

One concern I have is not to result in a big mix of some methods utilizing callbacks and some returning promises.

cc: @MarshallOfSound

This comment has been minimized.

@sethlu

sethlu May 28, 2018

Member

Actually I just realized .stopLogging([callback]) can return void when callback is provided, otherwise a promise. I'm not sure if that overloads too much complexity for the API.

This comment has been minimized.

@MarshallOfSound

MarshallOfSound May 28, 2018

Member

I've discussed this in many places, basically until we have a good consistent story for promises in cpp land I want to stick with callbacks. My work on promise helpers in cpp has been stilted somewhat due to other more pressing concerns.

@deepak1556

Agree with @MarshallOfSound about not adding it to the net module, I would suggest adding the api under app module, as netlog is tied to the lifetime of the app.

/**
* Starts logging to the filepath remembered, otherwise to the default specified
* via --log-net-log.
*/
void NetLog::StartLogging() {

This comment has been minimized.

@deepak1556

deepak1556 May 28, 2018

Member

Starts logging to the filepath remembered, otherwise to the default specified via --log-net-log.

It would be better to not have any dependency between the switch and the api since both these invocations happens on different threads, avoids confusion and simplifies logic.

@sethlu

This comment has been minimized.

Member

sethlu commented May 29, 2018

@deepak1556 @MarshallOfSound

What do you think about the following spec? Also the CLI switch's functionality is decoupled from the programmable net log API. I'm not sure if it fits well under the app module though.

CLI (unchanged)

electron --log-net-log=filepath

API

electron.netLog.startLoggingToFile(filepath)
electron.netLog.isLoggingToFile // property
electron.netLog.stopLoggingToFile()

I'm now trying to model it similar to contentTracing's .{start|stop}Recording().

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented May 29, 2018

To clarify on things like return types and adjust a few things (all IMO)

electron.netLog.startLogging(filepath: string): void
electron.netLog.currentlyLogging: boolean
electron.netLog.stopLogging(cb?: Function): void
@sethlu

This comment has been minimized.

Member

sethlu commented May 29, 2018

@MarshallOfSound Yes, I agree with the types 馃憤

For the netLog.currentlyLogging would netLog.isCurrentlyLogging be better?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented May 29, 2018

IMO currentlyLogging is semantically is a state value, isCurrentlyLogging implies a query so would be a method not a getter.

@YurySolovyov

This comment has been minimized.

Contributor

YurySolovyov commented May 30, 2018

should there be

electron.netLog.currentlyLoggingPath: string

or

electron.netLog.getLoggingPath(): string

?

@sethlu

This comment has been minimized.

Member

sethlu commented May 30, 2018

@YurySolovyov Yea I can add electron.netLog.currentlyLoggingPath: string. The fallback case when it's not logging will just be an empty string.

@sethlu sethlu requested a review from electron/docs as a code owner May 30, 2018

sethlu added some commits May 30, 2018

@sethlu sethlu changed the title from [WIP] feat: Enhance net API for dynamic logging control to feat: Enhance net API for dynamic logging control Jun 1, 2018

@sethlu

This comment has been minimized.

Member

sethlu commented Jun 1, 2018

I'm wondering if there is a convention in Electron... that when logging is already started, should the API give a warning when .startLogging() is called again or throw an exception?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jun 2, 2018

@sethlu

Lot's of our API's return a boolean indicating success

@sethlu sethlu changed the title from feat: Enhance net API for dynamic logging control to feat: netLog API for dynamic logging control Jun 4, 2018

@sethlu

This comment has been minimized.

Member

sethlu commented Jun 5, 2018

@MarshallOfSound @deepak1556 This PR should be ready for review. Please let me know if there be any questions! Also I'm wondering if it's possible to back port the changes to 2-0-x once it's merged?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jun 5, 2018

Will review when I get a chance, currently at summit 馃槃

Also I'm wondering if it's possible to back port the changes to 2-0-x once it's merged?

Not to 2-0-x, it could go into 2-1-x (I think it's not breaking) but would be quicker to go out in 3.0.0

@sethlu

This comment has been minimized.

Member

sethlu commented Jun 5, 2018

Right, to 2-1-x 馃槀, thanks by the way.
Hope the summit goes well!

@MarshallOfSound

had some pending comments

@@ -0,0 +1,90 @@
// Copyright (c) 2018 Slack Technologies, Inc.

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Jun 15, 2018

Member

This should be GitHub (c)? 馃槃

cc @felixrieseberg says so 馃槅

NetLog::~NetLog() {}
// static
v8::Local<v8::Value> NetLog::Create(v8::Isolate* isolate) {

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Jun 15, 2018

Member

Shouldn't this return mate::Handle?

This comment has been minimized.

@deepak1556

deepak1556 Jun 15, 2018

Member

There is an explicit call to ToV8 below.

void NetLog::StartLogging(mate::Arguments* args) {
base::FilePath log_path;
if (!args->GetNext(&log_path) || log_path.empty()) {
args->ThrowError("Valid path is required");

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Jun 15, 2018

Member

This error message can be more descriptive "The first parameter must be a valid string"

base::OnceClosure callback;
args->GetNext(&callback);
net_log_->StopDynamicLogging(std::move(callback));

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Jun 15, 2018

Member

I think we should be using base::Bind to move callbacks 馃 Not 100% on that usage though

This comment has been minimized.

@deepak1556

deepak1556 Jun 15, 2018

Member

base::Bind is only required when binding bare functions, it doesn't handle ownership of the callback, only the bound arguments.

@@ -0,0 +1,42 @@
// Copyright (c) 2018 Slack Technologies, Inc.

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Jun 15, 2018

Member

This one too

const {netLog, NetLog} = process.atomBinding('net_log')
NetLog.prototype.stopLogging = function (callback) {

This comment has been minimized.

@deepak1556

deepak1556 Jun 15, 2018

Member

Any reason why this check isn't performed in a similar way to StartLogging api using mate::Arguments ?

This comment has been minimized.

@sethlu

sethlu Jun 18, 2018

Member

Since the net log interface in Chromium for stop observing net logs takes base::OnceClosure, more work is needed to pass a callback function (which takes in a filepath) and wrap it in another closure to pass to Chromium.

NetLog.prototype.stopLogging(callback: (text: string) => void): void
NetLog.prototype._stopLogging(callback: () => void): void
@sethlu

This comment has been minimized.

Member

sethlu commented Jun 18, 2018

@MarshallOfSound @deepak1556

Thanks for reviewing! Are there any outstanding issues that need fixing?

return net_log_->IsDynamicLogging();
}
base::string16 NetLog::GetCurrentlyLoggingPath() {

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Jun 19, 2018

Member

This should just return base::FilePath::StringType, we have a converter that does the platform logic

This comment has been minimized.

@sethlu

sethlu Jun 19, 2018

Member

@MarshallOfSound 馃帀 Thanks! I didn't find out about this earlier!

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jun 19, 2018

Merging this in to get in before we branch off, I think all your feedback is covered @deepak1556 if not we can address in a follow up, just want to get this in for @sethlu so we don't have to wait another release cycle 馃憤

comments addressed, see my comment for merge reasoning

@MarshallOfSound MarshallOfSound merged commit ab24a1e into master Jun 19, 2018

11 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@MarshallOfSound MarshallOfSound deleted the net-log-api branch Jun 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment