search

livepeer / catalyst-api

MIT License
4 stars 2 forks source link

analytics.go: Fix typo in error message formatting #1352

Closed hjpotter92 closed 2 months ago

hjpotter92 commented 3 months ago

i was getting tired of seeing %v appear in logs:

W0731 05:15:49.119430 2566432 analytics.go:117] cannot parse geo info from analytics log request header, err=%vmissing geo headers: [X-Region-Name]
pwilczynskiclearcode commented 3 months ago

We're flooding the logs with these warnings/errors. It's mostly X-Region-Name missing... maybe we could live without Region and stop spamming the logs

hjpotter92 commented 3 months ago

We're flooding the logs with these warnings/errors. It's mostly X-Region-Name missing... maybe we could live without Region and stop spamming the logs

yeah, probably might be better if we only log for cases when multiple headers are missing (for eg.):

W0731 09:48:49.223187 2835021 analytics.go:117] cannot parse geo info from analytics log request header, err=%vmissing geo headers: [X-City-Country-Name X-City-Country-Code X-Region-Name]
pwilczynskiclearcode commented 3 months ago

We're flooding the logs with these warnings/errors. It's mostly X-Region-Name missing... maybe we could live without Region and stop spamming the logs

yeah, probably might be better if we only log for cases when multiple headers are missing (for eg.):

W0731 09:48:49.223187 2835021 analytics.go:117] cannot parse geo info from analytics log request header, err=%vmissing geo headers: [X-City-Country-Name X-City-Country-Code X-Region-Name]

There are no cases of missing headers other than X-Region-Name so either excluding just this header from the warning or reporting only if more than one is missing like you suggested should be good.

thomshutt commented 3 months ago

@ecmulli is this header missing expected or something we need to investigate? If it's expected then agree we should stop spamming the logs about it

pwilczynskiclearcode commented 3 months ago

I think that geoip does not always provide this field

ecmulli commented 3 months ago

@ecmulli is this header missing expected or something we need to investigate? If it's expected then agree we should stop spamming the logs about it

It is not expected but I like @pwilczynskiclearcode 's suggestion of only logging when multiple are missing.

leszko commented 2 months ago

@ecmulli is this header missing expected or something we need to investigate? If it's expected then agree we should stop spamming the logs about it

It is not expected but I like @pwilczynskiclearcode 's suggestion of only logging when multiple are missing.

I think we should not log when X-Region-Name is missing (as suggested by Paweł). We expect this field sometimes not being present. @hjpotter92 I think you can merge your PR and I'll fire a new one with this logic.