FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat: implement logger by exaby73 · Pull Request #121 · firebase/firebase-functions-python · GitHub

feat: implement logger - #121

Merged
taeold merged 12 commits into
mainfrom
feat/logger
Oct 31, 2023
Merged

feat: implement logger#121
taeold merged 12 commits into
mainfrom
feat/logger

Conversation

exaby73 commented Jul 18, 2023

Copy link
Copy Markdown
Contributor

Closes #79

exaby73 marked this pull request as ready for review July 19, 2023 11:32
Salakar changed the title Implement logger feat: implement logger Aug 4, 2023

exaby73 commented Sep 25, 2023

Copy link
Copy Markdown
Contributor Author

Demo of logging

Sample code
from firebase_functions import https_fn, logger, options
from firebase_admin import initialize_app

options.set_global_options(max_instances=10)

initialize_app()


@https_fn.on_request()
def on_request_example(req: https_fn.Request) -> https_fn.Response:
    args = req.args
    logger.log(message=f'Log from function: ', args=args)
    return https_fn.Response('OK')

After deployment via firebase deploy --only functions, you paste the generated link in your browser and add any query params which will be translated into the args variable used in the function.

For example: https://<generated>.run.app?foo=bar

Result in Cloud Logging

Note: when selecting the Cloud Function from Cloud Logging, it generates the following query:

resource.type="cloud_function"
resource.labels.function_name="on_request_example"

This unfortunately doesn't show our logs. Instead, if you select the service name (same as function name) from Cloud Run Revision, it'll generate the following query:

resource.type="cloud_run_revision"
resource.labels.service_name="on-request-example"

Alternatively, you could directly paste this query, replacing resource.labels.service_name

taeold commented Sep 28, 2023

Copy link
Copy Markdown
Contributor

q: why does the example you shared in #121 (comment) emit a textPayload not a jsonPayload as I'd expect when you give the log a structured data like a dictionary?

exaby73 commented Oct 3, 2023

Copy link
Copy Markdown
Contributor Author

@taeold The code was adapted from the node logger

taeold commented Oct 10, 2023
edited
Loading

Copy link
Copy Markdown
Contributor

@exaby73 I'm not sure I understand what you mean. The purpose of nodejs logger is so that it's easy to emit structured log entry from valid JS object and not to emit them as just text message.

e.g.

exports.helloLogs = functions.https.onRequest(async (request, response) => {
  functions.logger.warn("Hello wolrd!", { foo: "bar" });
  response.send("hello world");
});

If the python logger isn't doing any work to generate structued logs using jsonPayload and can't set appropriate logging levels that corresponds to logging levels in Cloud Logging, then it's no more useful than print() calls.

Can you confirm if:

  1. Python logger is capable of emitting structured logs based on dictionary input
  2. Python logger sets correct logging level

exaby73 commented Oct 19, 2023
edited
Loading

Copy link
Copy Markdown
Contributor Author

@taeold I've updated the logger to support jsonPayload. The *args will be joined with a space to produce the message key and the **kwargs will be spread into the jsonPayload. Here is the result of calling logger.log("Hello World!", foo="Bar"):

uid=token_data["uid"],
email=token_data.get("email"),
email_verified=token_data.get("email_verified"),
email_verified=bool(token_data.get("email_verified")),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

hmm this feels unrelated? should we make a separate PR for this or is this necessary for log impl?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

mypy was updated and was causing CI to fail because the types didn't match. I'm open to suggestions. This is mainly a linting fix

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

maybe clearest to make the mypy related linting fixes in a separate PR, and have only the logging things here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Without these, the CI doesn't pass. Possibly can make a separate PR for these that can merged into main, before we merge this in so that we can have those changes rebased into this branch. I'll look into doing this next week :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

eh let's save ourselves some time and push it in this PR.

elif precision_timestamp == PrecisionTimestamp.SECONDS:
return second_timestamp_conversion(time)

raise ValueError("Invalid timestamp")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

likewise - related to log impl?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Again a lint failure here because the function wasn't ending in a return and the if, elif is not exhaustive

uid=token_data["uid"],
email=token_data.get("email"),
email_verified=token_data.get("email_verified"),
email_verified=bool(token_data.get("email_verified")),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

eh let's save ourselves some time and push it in this PR.

taeold merged commit 90211ab into main Oct 31, 2023

taeold commented Oct 31, 2023

Copy link
Copy Markdown
Contributor

@exaby73 thank you!!

exaby73 deleted the feat/logger branch October 31, 2023 16:22
jaclar mentioned this pull request Nov 23, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

Add a logger module to integrate with Cloud Logging

3 participants


Back | FazBrowse Home | New Git URL