Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

logData; logError #16

Closed
wants to merge 1 commit into from
Closed

logData; logError #16

wants to merge 1 commit into from

Conversation

devoncarew
Copy link
Contributor

Not for landing, but perhaps food for thought; this PR adds a data channel (logData) w/ the ability to generate a custom message to display for the log. Similarly with the logError call. Both try to delay work until after we've checked whether logging is enabled for that channel.

A final solution likely wouldn't have the same API but may have similar functionality (data, message, and error logging?).

@pq
Copy link
Owner

pq commented Sep 28, 2018

I think I like this!

Can you maybe show some examples of this in action?

@devoncarew
Copy link
Contributor Author

I've iterated a bit on this (or away from from this). Mostly what I'm thinking of is:

  • we don't json encode the message
  • the message is meant to be user presentable
  • we have a way to log an error, which uses the error field in the developer log method, and sets the level field to 1000 (severe)
  • we have a way to log a data object. that does use the message field in order to convey a user presentable message. The data object itself is converted to json when sent, and the jsonified string value is what's sent as the data payload; we use the error field for this

This gets us user-presentable logging messages, and a structured data which is easy to unpack on the client as essentially a Map of values. Happy to talk more in person :)

@pq
Copy link
Owner

pq commented Sep 28, 2018

Got it. We should definitely discuss.

I wonder if we couldn't do something simple w/ a single function that might handle all the cases:

log(() => 'hello');
log(() => 'an error occurred', severity: 1000, data: errorObject);
// or maybe:
log(() => 'an error occurred', kind: EventKind.error, data: errorObject);
log(()=> 'got data', data: dataObject);

with maybe some convenience added to the Log wrapper:

class Log {
  void error(MessageCallback messageCallback, { Object errorData}) {
     log(messageCallback, kind: EventKind.error, data: errorData);
  }
}

Thinking about event kinds, are there more than error and data?

Also, how do you feel about sequence numbers? Would they come in handy in any of your experiments?

@devoncarew
Copy link
Contributor Author

log(() => 'an error occurred', severity: 1000, data: errorObject);

That would work, but would need more client knowledge of the conventions.

Also, how do you feel about sequence numbers? Would they come in handy in any of your experiments?

I don't think they'd be generally useful - probably just detritus left over from closely mirroring the package:logging API. For network calls you might want to prefix a set of calls with an http sequence number, but there'd be several of a sequence logged.

@devoncarew
Copy link
Contributor Author

(closing as this wasn't intended to land...)

@devoncarew devoncarew closed this Sep 28, 2018
@pq pq mentioned this pull request Sep 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants