Add record's __dict__ to structPayload

[vmarkovtsev]

Not sure if it is not included intentionally... This is a light version of what is done at https://github.com/severb/graypy/blob/master/graypy/handler.py#L97

[dhermes]

@waprin Can you review this?

[dhermes]

@vmarkovtsev See the failing tests. This likely will also need at least one new unit test. I'm happy to help if need be.

But we can hold off on tests until @waprin vets the usefulness of this code change.

[vmarkovtsev]

Wow, cool! I didn't expect you would actually agree with this PR. I will fix the tests, code and add the new one for sure, thanks.

[dhermes]

@vmarkovtsev I didn't say I agree with it; I'm staying hands off until I hear from @waprin

[waprin]

@vmarkovtsev thanks for the PR, on the fence about this one but in general we want external contributions.

So a few issues I see:

1. This only addresses one transport. Currently there are two, this one and sync.py, and more will be added. They both inherit from a base class, and if we add this it should be generalized across the transports using the base class somehow.

2. What exactly is the justification for this? In the case of errors, we plan to add line number/file/stack trace to the Error Reporting API. Basically hook into this when severity is an error. It's not clear it's something we want to add to every log message even if it's not an error. If you could provide specific examples, it might help.

3. This actually works differently than the example you linked to. It looks like greypy explicitly includes fields, then explicitly filters out most of the fields from the dict and only adds "extra" attributes add_extra_fields. I am not sure what type of extra fields they are expecting to find in the LogRecord (do you know)?. Either way, I would rather explicitly add thread id/process ID then add everything in the dict and filter out a blacklist.

[dhermes]

Either way, I would rather explicitly add thread id/process ID then add everything in the dict and filter out a blacklist.

Ditto

[vmarkovtsev]

OK, this is my next attempt. If you say that it's not too radical I will fix the tests.

[vmarkovtsev]

BTW, extra_fields are as follows. You can add your custom LogRecord members using logging.Filter. E.g., I use the following code:

import logging
import socket


class HostnameFilter(logging.Filter):
    hostname = socket.gethostname()

    def filter(self, record):
        record.hostname = self.hostname
        return True

handler = CloudLoggingHandler(gcloud.logging.Client(), extra_fields=('hostname',))
handler.addFilter(HostnameFilter())

Filters are not required to add new fields, they can delete, change, etc.

[vmarkovtsev]

I can make this "data" thing optional and turned off by default if you force "this adds overhead" thesis.

My point is that from my experince with other log servers (e.g. GrayLog), logs are sent in a structured way so that you can build precise queries like "show all logs written in that file between lines 30 and 60". I am aware of the fact that the current gcloud web interface does not support it (am I right?).

[waprin]

Hey sorry I missed these updates, will review tomorrow

[waprin]

Hey, thanks for the good example, it makes sense to me. We will want something like that added in the usage doc.

The Cloud Console does support those kinds of filters, but it's still migrating from the v1 filter syntax to v2 filter syntax. In the meantime you can use the v2 syntax programmatically.

I'm not super concerned about overhead, just wanted to list things we should at least consider.

Overall I think you've justified why the change makes sense and I at least am happy to accept it (it's my feature branch but you will want signoff from one of the repo owners before merge).

Do be aware that we need Python 2.7 and Python 3 support, so some things like itertools might require six.

Thanks again, look forward to seeing Travis pass so we can merge.

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

[vmarkovtsev]

Well, I had to merge with master because they dropped Python 2.6 and I have dict comprehension. But it screwed SLA (wtf) and I have no idea why Travis has problems.

[dhermes]

The CLA fail is likely because the merged in commits have authors other than you @vmarkovtsev. You should rebase or cherry-pick instead of merging and then force-push over this branch

[vmarkovtsev]

@dhermes Don't you say! And what's this? 17251f4

[waprin]

@dhermes I think the issue is that @tseaver suggested using a feature branch but we never actually decided on a workflow that made sense unless you can give me write access to the feature branch, and I don't think Github has tha granular level of control. I could have developed the branch on my own repo but I wanted to make sure I was getting review at incremental points (and the fact that it was also part of this repo was helpful in getting some extra eyes on it like we're seeing in this PR getting made).

The problem we keep seeing is that we need a repo owner to merge the master branch back into this one. Otherwise, if you rebase on top of master but try to merge into this branch you get all those extra commits. @vmarkovtsev can't force push to the branch because again he doesn't have access.

Maybe the easiest thing to do is stick with personal repos for feature branch work and just ping you guys for reviews on it. In any case I would like to merge this feature branch into master soon anyway. There are lots of iterative improvements I would still like to make but none in my head will involve breaking changes. I wanted to add the fluentd handler but it's not even strictly necessary and will be a small non-breaking change. But if you'd really like me to wrap that up first it's no prob.

I asked @tseaver to merge my last PR for this branch but he had a few comments I still need to address and we should try to get this contribution merged as well. Then I'm wondering if there's any objection to just merging to master even if more work is queued up behind it.

[waprin]

@dhermes @tseaver another potential option: add me as collaborator to repo but setup master as a protected branch which you guys can just keep yourself on:

https://help.github.com/articles/configuring-protected-branches/

[waprin]

Though the easiest and most straightforward approach may be to just ask you to merge in master every now and then, and if there's a conflict we can submit it ourselves as a PR then rebase out other PRs on top of that.

Food for thought on how you might do this type of stuff going forward, in any case a merge of master into this branch now would be helpful.

[dhermes]

@waprin I can just

git co logging-stdlib-handler-feature
git rebase master

and deal with conflicts. Have you tried it? Are there issues?

[dhermes]

@waprin I just got your branch up to date for you.

[waprin]

Thanks. I didn't try in this case and would have to wait till we get out of date to try again, but I did it in a previous PR. If I rebase on master, then my PR will be based on master so will have all those commits that origin/std-logging-feature doesn't, so when I make the PR all those master commits are included as part of the PR which we don't want. We really want to rebase the entire feature branch on master, or merge master into the feature branch, so when the PR is made you only see the actually new commits. But in order to rebase origin/std-logging-feature onto master, we need someone with write access to origin. Hopefully that makes sense.

[dhermes]

@waprin Just file an issue if you need your branch gardened