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

feat(gcf-utils): add trigger info as bindings to all log statements #796

Merged
merged 8 commits into from
Aug 5, 2020

Conversation

azizsonawalla
Copy link
Contributor

@azizsonawalla azizsonawalla commented Aug 3, 2020

  • Adds support for initializing GCFLogger with bindings
  • Adds support for child loggers (with bindings) to GCFLogger
  • Uses above mentioned child logger functionality to bind trigger information to all log statements for an execution

Resulting log statements will look something like this:

(in gcf-utils.ts)

const triggerInfo  = {
    trigger: {
        trigger_type: 'GITHUB_WEBHOOK',
        trigger_sender: 'some sender',
        payload_hash: '123456',
        trigger_source_repo: {
            owner: 'foo owner',
            owner_type: 'Org',
            repo_name: 'bar name',
            url: 'some url',
        },
    },
};

...

bindTriggerInfoToLogger(triggerInfo);
(in some-bot.ts)

// after gcf-utils has done all the trigger parsing and logged the trigger information the first time

logger.info({ some-property: 'some-value' });
(Cloud Logs Viewer)

{
    insertId: "xyz",
    jsonPayload: {
        some-property: "some-value",                // what the bot logged
        trigger: {                                   // what was automatically attached to logs
            trigger_type: 'GITHUB_WEBHOOK',
            trigger_sender: 'some sender',
            payload_hash: '123456',
            trigger_source_repo: {
                owner: 'foo owner',
                owner_type: 'Org',
                repo_name: 'bar name',
                url: 'some url',
            },
        },
    },
    resource: {
        type: "cloud-function",
        labels: {
            function_name: "some-bot",
            project_id: "repo-automation-bots",
            region: "us-central1"
        },
    },
    timestamp: "12345",
    severity: "INFO",
    labels: {
        execution_id: "abcd"
    }
}

Fixes #777

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 3, 2020
@azizsonawalla azizsonawalla requested review from chingor13 and bcoe August 3, 2020 20:34
@azizsonawalla azizsonawalla marked this pull request as ready for review August 3, 2020 20:34
@azizsonawalla azizsonawalla requested a review from a team as a code owner August 3, 2020 20:34
bcoe
bcoe previously requested changes Aug 3, 2020
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, could you give me an example in the issue body of what the log message ends up looking like?

@azizsonawalla azizsonawalla requested a review from bcoe August 4, 2020 13:50
Comment on lines 216 to 220
const triggerInfo = buildTriggerInfo(triggerType, id, request.body);
logger.metric(triggerInfo);

delete triggerInfo.message; // we don't want to bind the message to every log entry
GCFBootstrapper.bindPropertiesToLogger(triggerInfo);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make buildTriggerInfo not include message, set the logger to the child logger, then do logger.metric("message originally in buildTriggerInfo")?

Copy link
Contributor Author

@azizsonawalla azizsonawalla Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean like this?

const triggerInfo = buildTriggerInfo(triggerType, id, request.body);
GCFBootstrapper.bindPropertiesToLogger(triggerInfo); // no message in triggerInfo

logger.metric({ 
  message: `Execution started by ${triggerType}`, 
  ...triggerInfo
});

This works too!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the trigger info should already be added to all the log entries, you should only have to call

logger.metric(`Execution started by ${triggerType}`);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh ok ok - makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

@azizsonawalla azizsonawalla requested a review from chingor13 August 4, 2020 17:36
@gcf-merge-on-green
Copy link
Contributor

Your PR has attempted to merge for 3 hours. Please check that all required checks have passed, you have an automerge label, and that all your reviewers have approved the PR

@chingor13 chingor13 merged commit 157c768 into googleapis:master Aug 5, 2020
gcf-merge-on-green bot pushed a commit that referenced this pull request Aug 5, 2020
azizsonawalla added a commit to azizsonawalla/repo-automation-bots that referenced this pull request Aug 6, 2020
chingor13 pushed a commit that referenced this pull request Aug 6, 2020
* Revert "feat(gcf-utils): add trigger info as bindings to all log statements (#796)"

This reverts commit 157c768.

* keep JSDocs and message changes
azizsonawalla added a commit to azizsonawalla/repo-automation-bots that referenced this pull request Aug 11, 2020
…oogleapis#796)

* feat(gcf-utils): add trigger info as bindings to all log statements

* don't recreate logger to attach bindings

* move deletion of message prop to main gcf method

* move trigger message addition to gcf-bootstrapper

* remove extra trigger info logging

Co-authored-by: Jeff Ching <chingor@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(gcf-utils): logger should keep scope for all log statements
3 participants