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

Encode compound types. #1

Open
ChrisHines opened this issue May 26, 2015 · 10 comments
Open

Encode compound types. #1

ChrisHines opened this issue May 26, 2015 · 10 comments

Comments

@ChrisHines
Copy link
Member

At the moment this package does not support logging values of type array, chan, func, map, slice, or struct. Of these, I cannot think of a reason to support chan or func. But I think we need a good story for array, map, slice, and struct.

Precedents

Brandur Leach's logfmt blog post does not explicitly explain how to represent compound types in logfmt, nor does the defacto logfmt definition at github.com/kr/logfmt. There are some ideas in an old Gist by Kieth Rarick. The Gist shows this JSON data:

{"dyno":"web", "metadata":{"stack":["line 1", "line 2"], "msg":"null"}}

converted to logfmt as follows.

dyno=web metadata={stack=["line 1" "line 2"] msg="null"}

But the parser at github.com/kr/logfmt does not handle this syntax.

Logrus relies on Go's %v format in the fmt package and writes:

dyno=web metadata=map[stack:[line 1 line 2] msg:null]

log15 uses Go's %+v format and writes:

dyno=web metadata="map[msg:null stack:[line 1 line 2]]"

Neither of these logging packages produce output that can be easily parsed back into the original data structure.

I have not investigated what the non-Go logfmt libraries do.

Ways Forward

Given that there is not a clear best practice for encoding compound data structures in logfmt, this package must either refuse to handle them (as it does now) or define a robust encoding for these data types.

@kr, @bmizerany, @inconshreveable, @sirupsen: I would appreciate it if any of you would like to comment on this issue.

@kr
Copy link

kr commented May 26, 2015

I suggest not supporting compound data in a log message. It is much easier conceptually (and as a practical matter when writing tools to process logs) if each log message is a flat dictionary. I had not yet learned this lesson when I wrote that old gist, but experience has taught me better in the intervening years.

@sirupsen
Copy link

An idea that's come up several time from Logrus users has been to implement a logger interface e.g. String() or Event() that Logrus would call.

@ChrisHines
Copy link
Member Author

@kr I am sympathetic to your suggestion, and that is the position I would take if there isn't an obviously good alternative. What do you think of flattening the data structures? For example:

dyno=web metadata.stack[0]="line 1" metadata.stack[1]="line 2" metadata.msg="null"

@sirupsen This package supports encoding.TextMarshaller and fmt.Stringer already, so those could serve that purpose. Do you think a logging specific interface would be better?

@basvanbeek
Copy link

I think it can be very valuable to have compound data be supported in log messages. I also have noticed that in most cases I don't need to have easy access to this data for the entire log but in those odd occasions that I had to find the proverbial needle in the haystack I've been extremely happy if every single detail was logged.

Depending on the application and type of data being logged I'd either not care at all or must have this data. Therefore I'd suggest adding the option to have serializer plugins for this data. These could be selected as default for all payloads being logged or specifically selected for a single log payload in the case that a particular log statement requires this highest level of detail at the expense of being slower.

Why not be able to have a compound data structure value be serialized to JSON in a Logfmt payload. Only when needed you'd deserialize this specific item.

By adding serializer plugins we still conform to simple flat key value pairs as the complex data is packaged into a string

@ChrisHines
Copy link
Member Author

@basvanbeek I agree that there can be value in supporting compound data. Can you elaborate on your plugin suggestion. It sounds like more complexity than I want in this package, but maybe there is a simple way I haven't thought of.

I want to take a moment to write down my current thoughts on this topic.

We log things to help us diagnose problems (among other uses). At the same time, most application code will not check errors returned from logging code. Even if a program does check and gets an ErrUnsupportedType, what should it do? There is a chance to catch the problem during test—if the error path is tested.

Which means that, at least for now, logfmt.MarshalKeyvals will replace unsupported values with an error message instead of returning an error and dropping the rest of the keyvals on the floor. Meanwhile, logfmt.Encoder will return errors because it is a lower level abstraction and code calling EncodeKeyval has enough context to handle the errors for each key/value pair individually. Code that wants to log compound values is required to make sure that those values are converted to a supported type before passed to the logfmt package.

The encoding portion of this package is targeted as a reusable piece for higher level structured logging packages such as logrus, log15, or gokit/log. Such packages can choose to handle compound values in a more sophisticated way.

If a good approach to compound values emerges it can still be incorporated in this package in the future.

@inconshreveable
Copy link

After thinking about this for a while, I believe that you've made the right decision: the logfmt package should not take a position on how to encode complex data.

log15's behavior could be implemented on top of the current API like so:

err := e.EncodeKeyval(k, v)
if err == logfmt.ErrUnsupportedValueType {
    e.EncodeKeyval(k, fmt.Sprintf("%+v", v))
}

@ChrisHines
Copy link
Member Author

@inconshreveable Thanks for taking the time to comment.

I think that in the short term it is best to avoid taking a position, but in the long term I would like to have a better story. The primary alternatives for structured logging formats (that I am aware of) seem to be JSON and logfmt. Several logging packages make it trivial to switch between the two formats. It would be nice if the two formats had the same level of support for data structures.

My vision for this package is that it eventually have feature parity with encoding/json.

@tgulacsi
Copy link

tgulacsi commented Sep 5, 2015

Json-marshal them wirhout blanks?

@ChrisHines
Copy link
Member Author

I have a few objections to using JSON:

  • It discards the benefit of human readability that logfmt has over JSON.
  • It would be unfortunate to have logfmt depend on enocding/json.
  • JSON uses " (quotes) which will need escaping, and that requires the logfmt value is wrapped with quotes even if there are no spaces.

@akutz
Copy link

akutz commented Sep 8, 2015

@sirupsen You appear to have tagged me in a comment on this post, but said comment is missing when I came here to contribute to the discussion?

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

No branches or pull requests

7 participants