logging: reduce message size #1721

pongad · 2017-03-09T05:23:48Z

This commit is based on top of #1720 . Once we merge that, I'll rebase to make this PR easier to review.

This PR fixes #1712 , using the method suggested in the issue.

To un-WIP this PR (in rough dependency-order):

1. ~~Think of a better name for "native level"~~
2. ~~Native level and setLevel works from the same config. When we know what to call the former, give it its own config.~~
3. The config is too difficult to test, because it's too difficult to pass value in from test.
Maybe we need a config class? It can be pre-populated with the property values but let users override.
4. Native level should be documented

@garrettjonesgoogle @michaelbausor What do you think of 1 and 3?

no change in behavior

Fixes #1712.

coveralls · 2017-03-09T05:45:19Z

Changes Unknown when pulling f9f862c on pongad:logging-label into ** on GoogleCloudPlatform:master**.

michaelbausor · 2017-03-09T18:09:39Z

Overall this looks good to me. Re your points:

Naming is hard :-( I don't mind native level. Or perhaps request level?
Does this need its own config? It is strictly for performance optimization, so having it match the Level setting makes sense since this is the most granular level of log that will be processed by the logger. At the least we should discourage users from setting it manually unless they really do know that they generate more logs at a higher level.
In Add async/sync setting to logging handler #1716 I added getters for flush level and size, matching the Handler parent class for its settings. So you could add a getter and setter for native level. I think that pulling these into a config class makes sense in general, but we will still be left with the getters and setters on the Handler parent class.

garrettjonesgoogle · 2017-03-09T19:12:51Z

I think "native level" is hard to figure out. "request level" is better, but still potentially confusing. How about "default level"?

michaelbausor · 2017-03-09T21:14:25Z

I think the problem with "default level" is that it is too similar to "level", which is a property of the Handler object. Maybe "default request level", but it still sounds like this setting has semantic meaning, rather than being just a performance optimization.

garrettjonesgoogle · 2017-03-09T21:28:47Z

maybe "common request level".

pongad · 2017-03-10T00:19:04Z

Answers inline

Maybe "default request level", but it still sounds like this setting has semantic meaning, rather than being just a performance optimization

Maybe we should just call it optimizeForLevel? I think "common request level" still implies some kind of default.

Does this need its own config?

You made a good point that since it's purely optimization, it might not need one. We could document the .level config that logs at that severity might be sent more efficiently. Maybe open a tracking issue where users can comment if they need this as a config.

So you could add a getter and setter for native level

Doing this raises a question I don't have a good answer to yet.
Changing label changes the partition key used by bundling. Currently bundling creates a new thread for each partition key. So changing label midway feels a bit like a time bomb waiting to go off. I think we should add getter/setter, but only after we make bundling a little more resilient. (googleapis/gax-java#194 should help some.)

I think we should do is:
For this PR

Finish item 4, by setting native level to be the same as .level config
Finish item 3, by reusing @michaelbausor 's change in Add async/sync setting to logging handler #1716
This should automatically help some of our users, and our users don't need to do anything

Later

Harden bundling
Now that we can change the native level midway, it'd be easier to do items 1 and 2
This lets advanced users tweak the settings to fit their app just right

@michaelbausor @garrettjonesgoogle WDYT?

michaelbausor · 2017-03-10T00:31:02Z

SGTM. Even once we can change do 1 and 2, we may not want to give users that knob until we are sure they want it. I think the tracking issue you suggest is a good idea.

Re googleapis/gax-java#194 I am working on that change - will close out that PR and open a new one once it is ready for review.

This PR turns the hard coded config string into a map and a rendering function. This doesn't do anything straightaway, but will simplify #1721 .

also improve tests

pongad · 2017-03-13T05:58:24Z

@garrettjonesgoogle @michaelbausor PTAL

coveralls · 2017-03-13T06:07:25Z

Changes Unknown when pulling c6139d6 on pongad:logging-label into ** on GoogleCloudPlatform:master**.

garrettjonesgoogle

LGTM - handle my comments as you see fit.

google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingHandler.java

@@ -67,6 +68,12 @@
 *     {@code java.log}).
 * <li>{@code com.google.cloud.logging.LoggingHandler.level} specifies the default level for the
 *     handler (defaults to {@code Level.INFO}).
+ *     <p> This configuration also sets the "base severity level".
+ *     Logs with the same severity with the base could be more efficiently sent to Stackdriver.
+ *     The base severity defaults to the level of of handler or {@code Level.FINEST}


google-cloud-logging/src/main/java/com/google/cloud/logging/LoggingHandler.java

@@ -67,6 +68,12 @@
 *     {@code java.log}).
 * <li>{@code com.google.cloud.logging.LoggingHandler.level} specifies the default level for the
 *     handler (defaults to {@code Level.INFO}).
+ *     <p> This configuration also sets the "base severity level".


google-cloud-logging/src/test/java/com/google/cloud/logging/LoggingHandlerTest.java

@@ -149,8 +147,15 @@
          .put("resourceType", "testResourceType")
          .put("synchronicity", "SYNC")
          .build();
+  private static final ImmutableMap<String, String> NATIVE_SEVERITY_MAP =


michaelbausor · 2017-03-13T22:44:07Z

LGTM

pongad · 2017-03-14T00:45:53Z

@garrettjonesgoogle I replied to your comment. Wanted to make sure it's good with you before merging. PTAL.

coveralls · 2017-03-14T01:46:47Z

Changes Unknown when pulling 72316dc on pongad:logging-label into ** on GoogleCloudPlatform:master**.

garrettjonesgoogle · 2017-03-15T03:16:16Z

LGTM

garrettjonesgoogle · 2017-03-16T00:07:58Z

ship it

pongad · 2017-03-16T00:08:25Z

I'll hold until CI pass just to make sure :)

coveralls · 2017-03-16T01:51:52Z

Changes Unknown when pulling d8d2ca0 on pongad:logging-label into ** on GoogleCloudPlatform:master**.

coveralls · 2017-03-16T02:06:57Z

Changes Unknown when pulling 744391d on pongad:logging-label into ** on GoogleCloudPlatform:master**.

pongad added 2 commits March 9, 2017 16:09

logging: clean up test

eea697f

no change in behavior

logging: reduce message size

f9f862c

Fixes #1712.

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 9, 2017

pongad requested review from garrettjonesgoogle and michaelbausor March 9, 2017 05:23

pongad mentioned this pull request Mar 10, 2017

logging: don't hard code config string #1728

Merged

michaelbausor mentioned this pull request Mar 10, 2017

Large memory usage by gcloud-logging API. #1449

Closed

pongad added a commit that referenced this pull request Mar 12, 2017

logging: don't hard code config string (#1728)

3919d63

This PR turns the hard coded config string into a map and a rendering function. This doesn't do anything straightaway, but will simplify #1721 .

Merge branch 'master' into logging-label

2fd0e20

pongad mentioned this pull request Mar 13, 2017

Support configuration of base severity level #1740

Closed

pongad changed the title ~~WIP: logging: reduce message size~~ logging: reduce message size Mar 13, 2017

document "base severity level"

c6139d6

also improve tests

garrettjonesgoogle reviewed Mar 13, 2017

View reviewed changes

pr comment

72316dc

pongad added 2 commits March 16, 2017 10:47

remove from javadoc

d8d2ca0

doc in member variable

744391d

pongad merged commit fad0300 into googleapis:master Mar 16, 2017

pongad deleted the logging-label branch March 16, 2017 00:20

logging: reduce message size #1721

logging: reduce message size #1721

Uh oh!

Conversation

pongad commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Mar 9, 2017

Uh oh!

michaelbausor commented Mar 9, 2017

Uh oh!

garrettjonesgoogle commented Mar 9, 2017

Uh oh!

michaelbausor commented Mar 9, 2017

Uh oh!

garrettjonesgoogle commented Mar 9, 2017

Uh oh!

pongad commented Mar 10, 2017

Uh oh!

michaelbausor commented Mar 10, 2017

Uh oh!

pongad commented Mar 13, 2017

Uh oh!

coveralls commented Mar 13, 2017

Uh oh!

garrettjonesgoogle left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

michaelbausor commented Mar 13, 2017

Uh oh!

pongad commented Mar 14, 2017

Uh oh!

coveralls commented Mar 14, 2017

Uh oh!

garrettjonesgoogle commented Mar 15, 2017

Uh oh!

garrettjonesgoogle commented Mar 16, 2017

Uh oh!

pongad commented Mar 16, 2017

Uh oh!

coveralls commented Mar 16, 2017

Uh oh!

coveralls commented Mar 16, 2017

Uh oh!

Uh oh!

pongad commented Mar 9, 2017 •

edited

Loading