Rejuvenate log levels #7737

yiming-tang-cs · 2019-11-01T17:43:36Z

We appreciate your previous feedback and it's very helpful! Here's a reissue of #7170 with a new version of our tool. It takes a long time, sorry for waiting. The tool made many fewer transformations, however, there were a few that we removed that you did not agree with in the original PR. Again, we'd appreciate any feedback and are willing to make further changes if you wish to incorporate our PR into your project.

Settings

We have several analysis settings. We can vary these settings and rerun if you desire. The settings we are using in this pull request are:

Treat CONFIG levels as a category and not a traditional level, i.e., our tool ignores these log levels.
Never lower the logging level of logging statements within catch blocks.
Never lower the logging level of logging statements within if statements.
Never lower the logging level of logging statements containing certain (important) keywords.
Never raise the logging level of logging statements without particular keywords in their messages.
Avoid log wrapping by disregarding logging statements contained in if statements mentioning log levels.
The greatest number of commits from HEAD evaluated: 1000.
The head at the time of analysis was: a49fb60

By placing an X in the preceding checkbox, I verify that I have signed the Contributor License Agreement

shs96c

Thank you for the updated PR. I've added comments explaining why the levels are the way they are for those places where I feel that the tool has erred. If those are cleaned up, I'd be happy to merge the remaining changes :)

shs96c · 2019-11-05T12:59:21Z

java/client/test/org/openqa/selenium/testing/drivers/TestEdgeDriver.java

@@ -76,7 +76,7 @@ private static URL getServiceUrl() {
                .findFirst().orElseThrow(WebDriverException::new);

        service = (EdgeDriverService) builder.withVerbose(true).withLogFile(logFile.toFile()).build();
-        LOG.info("edgedriver will log to " + logFile);
+        LOG.fine("edgedriver will log to " + logFile);


This is deliberately at this level.

shs96c · 2019-11-05T12:59:57Z

java/client/src/org/openqa/selenium/devtools/Connection.java

@@ -80,7 +80,7 @@ public Connection(HttpClient client, String url) {
      serialized.put("sessionId", sessionId);
    }

-    LOG.info(JSON.toJson(serialized.build()));
+    LOG.finest(JSON.toJson(serialized.build()));


Right now this is experimental and deeply flaky. We left this at info to make debugging user reports a lot easier.

shs96c · 2019-11-05T13:00:18Z

java/client/test/org/openqa/selenium/build/BazelBuild.java

@@ -40,7 +40,7 @@ public void build(String target) {
    if (target == null || "".equals(target)) {
      throw new IllegalStateException("No targets specified");
    }
-    log.info("\nBuilding " + target + " ...");
+    log.finest("\nBuilding " + target + " ...");


This is to let people know that the tooling is doing something during a build. Please leave.

shs96c · 2019-11-05T13:00:41Z

java/server/src/com/thoughtworks/selenium/webdriven/WebDriverBackedSeleniumHandler.java

@@ -110,7 +110,7 @@ public HttpResponse execute(HttpRequest req) throws UncheckedIOException {
    StringBuilder printableArgs = new StringBuilder("[");
    Joiner.on(", ").appendTo(printableArgs, args);
    printableArgs.append("]");
-    LOG.info(String.format("Command request: %s%s on session %s", cmd, printableArgs, sessionId));
+    LOG.finest(String.format("Command request: %s%s on session %s", cmd, printableArgs, sessionId));


info was chosen deliberately to mirror the old behaviour that users expected.

shs96c · 2019-11-05T13:00:55Z

java/server/src/org/openqa/selenium/docker/Container.java

@@ -58,7 +58,7 @@ public void start() {
  public void stop(Duration timeout) {
    Objects.requireNonNull(timeout);

-    LOG.info("Stopping " + getId());
+    LOG.warning("Stopping " + getId());


It's not a warning. It's an informational message.

shs96c · 2019-11-05T13:01:32Z

java/server/src/org/openqa/selenium/docker/Docker.java

@@ -93,7 +93,7 @@ public Image pull(String name, String tag) {
      throw new WebDriverException("Unable to pull container: " + name);
    }

-    LOG.info(String.format("Pull of %s:%s complete", name, tag));
+    LOG.fine(String.format("Pull of %s:%s complete", name, tag));


Waiting for the pull takes a long time. This message informs the user that at least one of the images being pulled is available. Please leave.

shs96c · 2019-11-05T13:02:01Z

java/server/src/org/openqa/selenium/grid/node/config/NodeOptions.java

@@ -70,7 +70,7 @@ private void addSystemDrivers(
      Capabilities caps = info.getCanonicalCapabilities();
      builders.stream()
          .filter(builder -> builder.score(caps) > 0)
-          .peek(builder -> LOG.info(String.format("Adding %s %d times", caps, info.getMaximumSimultaneousSessions())))
+          .peek(builder -> LOG.finest(String.format("Adding %s %d times", caps, info.getMaximumSimultaneousSessions())))


This is an informational message that allows someone to read the console output and understand how the grid node is configured. Please leave.

shs96c · 2019-11-05T13:02:45Z

java/server/src/org/openqa/selenium/remote/server/SeleniumServer.java

@@ -74,7 +74,7 @@ private Routable getRcHandler(ActiveSessions sessions) {
          getClass().getClassLoader())
          .asSubclass(Routable.class);
      Constructor<? extends Routable> constructor = rcHandler.getConstructor(ActiveSessions.class);
-      LOG.info("Bound legacy RC support");
+      LOG.finest("Bound legacy RC support");


This informational message is important to users. Please leave.

shs96c · 2019-11-05T13:03:07Z

java/server/test/org/openqa/selenium/grid/distributor/DistributorTest.java

@@ -582,7 +582,7 @@ public void shouldPrioritizeHostsWithTheMostSlotsAvailableForASessionType() {
        );

        Session firefoxSession = distributor.newSession(createRequest(firefoxPayload)).getSession();
-        LOG.info(String.format("Firefox Session %d assigned to %s", i, chromeSession.getUri()));
+        LOG.finer(String.format("Firefox Session %d assigned to %s", i, chromeSession.getUri()));


Since this is in a test, I imagine that the choice of info level was deliberate.

yiming-tang-cs · 2019-11-05T19:46:48Z

Thank you for your in-depth feedback! We will discuss it and improve this PR!

yiming-tang-cs · 2019-11-05T21:30:59Z

Hi, I cleaned up all the transformations that you disagreed with. We appreciate your valuable feedback!

yiming-tang-cs · 2019-11-06T15:51:09Z

If there's anything we can do before merging the request, please let us know. Thanks!

yiming-tang-cs · 2019-11-20T23:01:44Z

@shs96c Is there anything else we can do to merge the request?

CLAassistant · 2019-11-23T16:52:38Z

All committers have signed the CLA.

yiming-tang-cs · 2019-11-25T17:14:12Z

The last two commits are only used to correct my email address to sign the CLA.

yiming-tang-cs · 2019-11-25T17:19:31Z

I also updated the branch (685f9a9) because the PR was showing that "This branch is out-of-date with the base branch".

yiming-tang-cs · 2019-12-18T19:31:56Z

@shs96c Can we get it merged? Are there any other things we can do?

shs96c

Thank you for nudging, and apologies for the delay getting this into the tree. Thank you!

yiming-tang-cs · 2020-02-18T17:36:18Z

Thank you so much!!

shs96c requested changes Nov 5, 2019

View reviewed changes

shs96c added the C-java label Nov 5, 2019

saledouble added 2 commits November 25, 2019 12:05

Rejuvenate log levels

0db95f7

clean up

6660595

yiming-tang-cs force-pushed the c_1000_ififcatchKeys branch from 930ad5d to 6660595 Compare November 25, 2019 17:09

Merge branch 'master' into c_1000_ififcatchKeys

685f9a9

Merge branch 'master' into c_1000_ififcatchKeys

33e5a14

shs96c approved these changes Feb 15, 2020

View reviewed changes

shs96c merged commit be6010c into SeleniumHQ:master Feb 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rejuvenate log levels #7737

Rejuvenate log levels #7737

yiming-tang-cs commented Nov 1, 2019 •

edited

Loading

shs96c left a comment

shs96c Nov 5, 2019

shs96c Nov 5, 2019

shs96c Nov 5, 2019

shs96c Nov 5, 2019

shs96c Nov 5, 2019

shs96c Nov 5, 2019

shs96c Nov 5, 2019

shs96c Nov 5, 2019

shs96c Nov 5, 2019

yiming-tang-cs commented Nov 5, 2019

yiming-tang-cs commented Nov 5, 2019

yiming-tang-cs commented Nov 6, 2019

yiming-tang-cs commented Nov 20, 2019

CLAassistant commented Nov 23, 2019 •

edited

Loading

yiming-tang-cs commented Nov 25, 2019

yiming-tang-cs commented Nov 25, 2019 •

edited

Loading

yiming-tang-cs commented Dec 18, 2019

shs96c left a comment

yiming-tang-cs commented Feb 18, 2020

Rejuvenate log levels #7737

Rejuvenate log levels #7737

Conversation

yiming-tang-cs commented Nov 1, 2019 • edited Loading

Settings

shs96c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yiming-tang-cs commented Nov 5, 2019

yiming-tang-cs commented Nov 5, 2019

yiming-tang-cs commented Nov 6, 2019

yiming-tang-cs commented Nov 20, 2019

CLAassistant commented Nov 23, 2019 • edited Loading

yiming-tang-cs commented Nov 25, 2019

yiming-tang-cs commented Nov 25, 2019 • edited Loading

yiming-tang-cs commented Dec 18, 2019

shs96c left a comment

Choose a reason for hiding this comment

yiming-tang-cs commented Feb 18, 2020

yiming-tang-cs commented Nov 1, 2019 •

edited

Loading

CLAassistant commented Nov 23, 2019 •

edited

Loading

yiming-tang-cs commented Nov 25, 2019 •

edited

Loading