add bindings method to logger instances

[yellowbrickc]

Step 1:
Expose tools.getChindings()
Register a symbol for the method
Reference the method in prototype.

[yellowbrickc]

As you suggested @mcollina I created the PR early. I have anyway no choice, I'm stocked :(

I tried to expose the method on every logger but I'm obviously overseeing something, this is what the tests tell me. And I'm still not happy about the naming, "chindings" are used already in prototype.child() to store the result as string.

[davidmarkclements]

just some initial feedback

[yellowbrickc]

I verified and you are absolutely right, the childBindings is available. Unfortunately it does not return the expected chindings because the stream.lastLogger does not exists at that point.
This means for me that a simple refactoring won't do it, @davidmarkclements is right, we must store and return the child object. Or:
we revert everything and create a method in proto.js which could transform instance[chindingsSym] in an object, eventually strip everything not being a child information and return it:

function childBindings () {
  const chindings = this[chindingsSym]
  var chindingsObject = (chindings.indexOf(',') === 0) ? `{${chindings.substr(1)}}` : `{${chindings}}`
  var bindings = JSON.parse(chindingsObject) || {}
  // eventually strip props as hostname and pid
  return bindings
}

What do you think?

[mcollina]

+1 to that proposal @yellowbrickc! I would add that we should cache that value as it's not going to change after usage.

I like it a lot for two reasons:

1. it ensures that the data is fully cloned and serialized before accessing it back.
2. it does not touch a hot path in this library, which is still painful to opimize.

[yellowbrickc]

Ok, than I will do this.
Regarding the caching, it could be dangerous. With this solution

var bindings = null
function childBindings () {
  if (!bindings) {
    const chindings = this[chindingsSym]
    var chindingsObject = (chindings.indexOf(',') === 0) ? `{${chindings.substr(1)}}` : `{${chindings}}`
    bindings = JSON.parse(chindingsObject) || {}
    // eventually strip props as hostname and pid
  }
  return bindings
}

will the second of these tests fail, the method still returns the object w/o {foo: 'bar'}

test('WIP childBindings are exposed on instance', async ({ isA }) => {
  const instance = pino()
  isA(instance.childBindings(), 'object')
})

test('WIP childBindings are exposed on every instance', async ({ match }) => {
  const instance = pino().child({ foo: 'bar' })
  match(instance.childBindings(), { foo: 'bar' })
})

[mcollina]

Let's leave caching to a later time :).

[davidmarkclements]

Quick check though: would a user expect the bindings to be cloned, or would a user expect to have the same object that was passed to child (referntially)?

[mcollina]

100% cloned.

[davidmarkclements]

Given an input API: logger.child(bindings)
And and output API logger.childBindings() => bindings

I kind of would expect the same object back, however we definitely should clone it because we need to uphold/promote the assumption that once set, the bindings are immutable. So we'll need to make docs clear on this.

@yellowbrickc if it makes sense to clone directly from the passed in binding object, (because that may make caching easier), use http://npm.im/rfdc for cloning

[yellowbrickc]

Sorry, I didn't see your last comments.
I just finished what we discussed (and discovered the LsCache, I suppose, this is what you meant earlier), but I didn't touch this subject.
I have reread the #560 and I tried to fulfill his issues/wishes. I didn't returned the whole object (neither the level, nor any serializers) just the "current state of the binding props for child loggers".

I tried but I cannot imagine any situation where I would need something else from a childlogger. But it is your decision :)

[davidmarkclements]

nice job so far!

[davidmarkclements]

I think this also has to be intergrated with pretty wrapper as well right? we need to remove that code from the pretty wrapper and use this instead?

[yellowbrickc]

The private method in the prettyWrapper does not remove any props, adds "v:1" to the output and is based on the lastLogger which is created/overwritten during the life cycle of the logger instance. It is used only for writing the logs.
The new method is a simple nice-to-have "reader" method, without side effects but with a try-catch (which I wouldn't us if errors wouldn't be allowed like in the other method) .
Also if you ask me than no, I wouldn't replace the other method.

[mcollina]

Good work!

[mcollina]

LGTM

[yellowbrickc]

Finally I passed your 100% code coverage requirement 😆
I cannot find any open conversations, I think @davidmarkclements you must accept them maybe?

[mcollina]

Thanks for keeping up with us @yellowbrickc! Good work!

[jsumners]

LGTM

[mcollina]

LGTM

[davidmarkclements]

sorry for the delay, I'm just thinking about the name,

if the logger is a child, then the returned object isn't child bindings, it's just bindings

I think we should rename childBindings to bindings

[yellowbrickc]

Done.

[davidmarkclements]

amazing thanks @yellowbrickc

[davidmarkclements]

v5.11.0 released 👍

[yellowbrickc]

It was a pleasure.
It would be very helpful for me, if we could have short "retro" ;-) I want to gather more experience in working on OSS.

1. At the end this was a very small PR (less then 20 lines) but we still had a lot of reworking and a lot of communication. My question is: this is the usual way or you would like more or less planing before?

2. Is this anything else what you would have to run somehow different?

[jsumners]

I suspect some of the back and forth was due to a small language barrier. Regardless if that is true or not, it’s not the ultimate delta in code size that dictates the conversation around its implementation. The goal of the conversation is to make sure the right code gets checked in.

[yellowbrickc]

I have no problem at all with back and forth communication. I rather build something what fulfills some needs than something else. This is why I liked the last change request from @davidmarkclements very much because it shows that you really care not only about coding but also about naming.

Maybe I can give my answers:

1. I don't know if this is the usual way in OSS but this is the usual way for me to build the right features.
2. I like the way how this PR evolved.
3. Just feedback from me regarding the repo: I made very good experience with a separate test file for edge cases. This is where I would start with recreating bugs and it would be not a unit test but a behavior test set.

BTW: thank you for the "fireworks" on twitter, I was really flattered - because we know that it wasn't that much what I did.

[mcollina]

Pino does not receive many contributions, and this was exceptionally well done.
This PR is an example of why OSS is awesome 👏 .

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.