Repository navigation
feat(handler): log iata.find failures in Register - #101
Conversation
If the requested service is "ndt7" or "ndt7_client", then the Autojoin API should not return, for example, Wehe servers. Prometheus (well, gcp-service-discovery) queries the Autojoin API for script-exporter targets for ndt7 e2e testing. The list endpoint should not return machines running experiments that are not compatible with the requested service.
See the change in this commit to understand what the comment was added for, which may not have previously been obvious.
When a node registers with an IATA code that is not present in the ip2location airport dataset (e.g. a metropolitan code like MIL rather than an airport code like LIN), Register returns a 500 with error type "iata.find". This branch was the only 500 path in Register that emitted no server-side log, so the failure was invisible in AppEngine logs and could only be diagnosed by reading the code and inspecting request URLs. Add a log line capturing the offending iata code, org, and service so future occurrences are identifiable directly from the logs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@nkinkade is this PR ready to be reviewed? |
|
@robertodauria: I think so. I apparently just forgot to ever ask for a review. |
|
@nkinkade what do you think of adding a Prometheus metric, too? |
|
@robertodauria: If we were to add a Prometheus metric, then it should probably be a generic one for all /register errors, with some label that helps to distinguish which one. But what would we use the metric for? Are you imagining an a new alert, or a dashboard panel showing errors? I'm not sure we need to the former, at least not yet, and I'm not sure how often I'd ever look at the latter. The main purpose of this PR was to help with troubleshooting specific /register failures, and a metric or dashboard probably wouldn't have helped. What do you think? |
robertodauria
left a comment
There was a problem hiding this comment.
My thinking is that we don't generally look at logs without any reason to do so, and a metric on a dashboard would at least let us know if there's something to check the logs for. I was imagining a generic metric for /register errors, with a label per type - it's the first thing I'd like to see in a hypothetical Autojoin API dashboard.
That said, ideas for the future shouldn't block current PRs. LGTM.
|
Ah, BTW: what's the dockercloud error about? |
|
Thanks for calling that out. It appears to have been a transient error: ... I restarted the build and it completed successfully. |
Summary
When a node registers with an IATA code that is not present in the ip2location airport dataset,
Registerreturns a 500 with error typeiata.find. This branch was the only 500 path inRegisterthat emitted no server-side log, so the failure was invisible in App Engine logs and could only be diagnosed by reading the code and inspecting raw request URLs.This adds a single log line capturing the offending IATA code, org, and service, so future occurrences are identifiable directly from the logs.
Motivation
A Milan autonode was switched from IATA
LIN(Milan-Linate airport) toMIL.MILis a valid IATA metropolitan code, but our dataset (ip2location-iata-icao.csv) is airport-keyed and contains no metropolitan codes, soiata.Find("mil")fails and the node gets a 500 on every registration attempt (observed: 323 consecutive 500s over ~14 days). Nothing appeared in App Engine logs because this error branch didn't log — diagnosing it required reading the handler source. TheType/Titlefields are returned to the client in the JSON body but never recorded server-side; this closes that observability gap.Change
Consistent with the existing
log.Println(...)calls in the same handler (dns register / tracker / service-account-key failures), with the IATA code, org, and service added so the log line is self-identifying.Testing
go build ./...passesgo test ./handler/ ./iata/passessandbox-kinkade(mlab-sandbox) green, including deploy🤖 Generated with Claude Code
This change is