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: Add metrics, gcp logging to tokenserver scripts #1555

Merged
merged 16 commits into from
May 23, 2024

Conversation

jrconlin
Copy link
Member

@jrconlin jrconlin commented May 3, 2024

Description

This adds GCP logging JSON output formatting for logs as well as DataDog style metric reporting.

New Environs:

  • METRIC_HOST - Collector Host address
  • METRIC_PORT - Collector Host port
  • METRIC_PATH - Socket path to collector (if available)

This also adds the --human arg if you want to use the older, human readable logs instead.

Issue(s)

Closes SYNC-4259

jrconlin added 2 commits May 2, 2024 16:22
This adds GCP logging JSON output formatting for logs as well as DataDog
style metric reporting.

New Environs:
* `METRIC_API_KEY` - Optional Datadog API key
* `METRIC_APP_KEY` - Optional Datadog APP key
* `METRIC_HOST` - Collector Host address
* `METRIC_PORT` - Collector Host port

This also adds the `--human` arg if you want to use the older, human
readable logs instead.
@pjenvey
Copy link
Member

pjenvey commented May 3, 2024

Why's datadog used (we switched off it a while ago)?

@jrconlin
Copy link
Member Author

jrconlin commented May 3, 2024

I thought that the metric collector was datadog compliant at least?
I used that library because I figured it was standard enough.

tools/tokenserver/process_account_events.py Outdated Show resolved Hide resolved
tools/tokenserver/purge_old_records.py Outdated Show resolved Hide resolved
tools/tokenserver/util.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
tools/tokenserver/util.py Outdated Show resolved Hide resolved
tools/tokenserver/util.py Outdated Show resolved Hide resolved
tools/tokenserver/purge_old_records.py Outdated Show resolved Hide resolved
tools/tokenserver/purge_old_records.py Outdated Show resolved Hide resolved
tools/tokenserver/requirements.txt Outdated Show resolved Hide resolved
# We need to reformat a few things to get the record to display correctly
# This includes "escaping" the message as well as converting the timestamp
# into a parsable format.
class GCP_JSON_Formatter(logging.Formatter):
Copy link
Member

@pjenvey pjenvey May 14, 2024

Choose a reason for hiding this comment

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

I'll note google provides a google-cloud-logging package. For some reason instead of a Formatter it provides a couple different handlers such as StructuredLogHandler for outputting to stdout. I gave it a quick try and it doesn't even include the timestamp from the log record (which is optional anyway, the log capturer probably just fills one in for the time it received it in that case). Just as an FYI: I was going to suggest switching to it but it's a bit odd? I'm fine with your version (though I suppose some of the extra metadata it adds could be handy later)

{"message": "Adding node foo to service sync-1.5","severity": "INFO", "logging.googleapis.com/labels": {"python_logger": "tokenserver.scripts.add_node"}, "logging.googleapis.com/trace": "", "logging.googleapis.com/spanId": "", "logging.googleapis.com/trace_sampled": false, "logging.googleapis.com/sourceLocation": {"line": 23, "file": "/home/pjenvey/src/moz/syncstorage-rs/tools/tokenserver/add_node.py", "function": "add_node"}, "httpRequest": {} }

{"message": "Error while adding node\nTraceback (most recent call last):\n File \"/home/pjenvey/src/moz/syncstorage-rs/tools/tokenserver/add_node.py\", line 25, in add_node\n database = Database()\n File \"/home/pjenvey/src/moz/syncstorage-rs/tools/tokenserver/database.py\", line 260, in __init__\n engine = create_engine(os.environ['SYNC_TOKENSERVER__DATABASE_URL'])\n File \"/usr/lib/python3.10/os.py\", line 680, in __getitem__\n raise KeyError(key) from None\nKeyError: 'SYNC_TOKENSERVER__DATABASE_URL'","severity": "ERROR", "logging.googleapis.com/labels": {"python_logger": "tokenserver.scripts.add_node"}, "logging.googleapis.com/trace": "", "logging.googleapis.com/spanId": "", "logging.googleapis.com/trace_sampled": false, "logging.googleapis.com/sourceLocation": {"line": 28, "file": "/home/pjenvey/src/moz/syncstorage-rs/tools/tokenserver/add_node.py", "function": "add_node"}, "httpRequest": {} }

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I figure we can subclass or monkey patch that one at some point.

Comment on lines 39 to 44
class Retry(requests.HTTPError):

def is_retryable(self):
self.response.status_code in [502, 503, 504]


Copy link
Member

Choose a reason for hiding this comment

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

Not used?

Suggested change
class Retry(requests.HTTPError):
def is_retryable(self):
self.response.status_code in [502, 503, 504]

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, thanks. Forgot to pull that.

tools/tokenserver/util.py Outdated Show resolved Hide resolved
pjenvey
pjenvey previously approved these changes May 22, 2024
tools/tokenserver/util.py Outdated Show resolved Hide resolved
Co-authored-by: Philip Jenvey <[email protected]>
@jrconlin jrconlin merged commit 6537783 into master May 23, 2024
8 checks passed
@jrconlin jrconlin deleted the feat/SYNC-4259_metrics branch May 23, 2024 01:19
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.

2 participants