Adding a "zone" value to "class LogRecord"

Adding a "zone" value to "class LogRecord"

R=sgjesse@google.com, wibling@google.com

Committed: https://github.com/dart-lang/logging/commit/0a994a926d502bda0060a2507fba48611886fb07

[kustermann, 2014-12-08 18:21:14]

kustermann@google.com changed reviewers:
+ sgjesse@google.com, wibling@google.com

[kustermann, 2014-12-08 18:31:39]

See here: https://github.com/dart-lang/appengine/issues/11

[Søren Gjesse, 2014-12-09 07:48:35]

lgtm

https://codereview.chromium.org/786813002/diff/1/dart/pkg/logging/lib/logging...
File dart/pkg/logging/lib/logging.dart (right):

https://codereview.chromium.org/786813002/diff/1/dart/pkg/logging/lib/logging...
dart/pkg/logging/lib/logging.dart:140: * be called on it and the result will be
logged.
Please update the comment with the information that the current zone is captured
- and maybe what the benefit of that information is.

https://codereview.chromium.org/786813002/diff/1/dart/pkg/logging/lib/logging...
dart/pkg/logging/lib/logging.dart:142: void log(Level logLevel, message, [Object
error, StackTrace stackTrace]) {
Would it make sense to make it possible to override zone here?

https://codereview.chromium.org/786813002/diff/1/dart/pkg/logging/lib/logging...
dart/pkg/logging/lib/logging.dart:320: LogRecord(this.level, this.message,
this.loggerName, [this.error,
I know that this is not related to you change, but couldn't this constructor be
private? I don't see any public method for adding a LogRecord.

https://codereview.chromium.org/786813002/diff/1/dart/pkg/logging/test/loggin...
File dart/pkg/logging/test/logging_test.dart (right):

https://codereview.chromium.org/786813002/diff/1/dart/pkg/logging/test/loggin...
dart/pkg/logging/test/logging_test.dart:181: expect(records.first.zone,
equals(recordingZone));
I assume this one would fail before.

[wibling, 2014-12-09 07:58:01]

lgtm

[kustermann, 2014-12-09 14:00:25]

So I wanted to consult Lasse on this as well. He has a bit of a negative feeling
about this and would rather have the zone value be private and a function for
looking up zone values.

I'm still not completely sure what to do.

[kustermann, 2015-01-07 12:36:36]

Patchset #2 (id:20001) has been deleted

[kustermann, 2015-01-07 12:38:11]

Patchset #2 (id:40001) has been deleted

[kustermann, 2015-01-07 12:38:45]

PING

https://codereview.chromium.org/786813002/diff/1/dart/pkg/logging/lib/logging...
File dart/pkg/logging/lib/logging.dart (right):

https://codereview.chromium.org/786813002/diff/1/dart/pkg/logging/lib/logging...
dart/pkg/logging/lib/logging.dart:140: * be called on it and the result will be
logged.
On 2014/12/09 07:48:35, Søren Gjesse wrote:
> Please update the comment with the information that the current zone is
captured
> - and maybe what the benefit of that information is.

Done.

https://codereview.chromium.org/786813002/diff/1/dart/pkg/logging/lib/logging...
dart/pkg/logging/lib/logging.dart:142: void log(Level logLevel, message, [Object
error, StackTrace stackTrace]) {
On 2014/12/09 07:48:35, Søren Gjesse wrote:
> Would it make sense to make it possible to override zone here?

Done.

https://codereview.chromium.org/786813002/diff/1/dart/pkg/logging/lib/logging...
dart/pkg/logging/lib/logging.dart:320: LogRecord(this.level, this.message,
this.loggerName, [this.error,
On 2014/12/09 07:48:35, Søren Gjesse wrote:
> I know that this is not related to you change, but couldn't this constructor
be
> private? I don't see any public method for adding a LogRecord.

That would be a breaking change and I don't intent to do this.

And also: It might be useful for others to use it (e.g. modify LogRecords), ...

https://codereview.chromium.org/786813002/diff/1/dart/pkg/logging/test/loggin...
File dart/pkg/logging/test/logging_test.dart (right):

https://codereview.chromium.org/786813002/diff/1/dart/pkg/logging/test/loggin...
dart/pkg/logging/test/logging_test.dart:181: expect(records.first.zone,
equals(recordingZone));
On 2014/12/09 07:48:35, Søren Gjesse wrote:
> I assume this one would fail before.

Can you explain what you mean here? Before my CL there was no 'zone' field.

What this ensures is that the zone at log.info() will record the zone the call
was made in -- even if in a child zone of root.

[Søren Gjesse, 2015-01-07 12:50:02]

lgtm

https://codereview.chromium.org/786813002/diff/1/dart/pkg/logging/test/loggin...
File dart/pkg/logging/test/logging_test.dart (right):

https://codereview.chromium.org/786813002/diff/1/dart/pkg/logging/test/loggin...
dart/pkg/logging/test/logging_test.dart:181: expect(records.first.zone,
equals(recordingZone));
On 2015/01/07 12:38:44, kustermann wrote:
> On 2014/12/09 07:48:35, Søren Gjesse wrote:
> > I assume this one would fail before.
> 
> Can you explain what you mean here? Before my CL there was no 'zone' field.
> 
> What this ensures is that the zone at log.info() will record the zone the call
> was made in -- even if in a child zone of root.

My comment didn't really make sense.

[kustermann, 2015-01-07 12:51:24]

Committed patchset #3 (id:80001) manually as
0a994a926d502bda0060a2507fba48611886fb07 (presubmit successful).