dewey 6 years ago

As this seems to be a beginner project to learn here's some thoughts on the code:

1) You should really not build your json by manually building the string. That's very error prone and hard to extend.

  fmt.Fprintf(f,
  "{\n\t\"from\": {\n\t\t\"name\": \"%s\",

Use https://golang.org/pkg/encoding/json/ (https://gobyexample.com/json) instead. Working with json in Go is really one of the nicer features of Go (It's Go, not "GoLang")

2) By setting / typoing a config option you could accidentally delete files from your computer, even your home directory

  err := os.Remove(jsonfile)
  • jerf 6 years ago

    A similar comment for the HTML portions of the message. Use html/template. It's got a couple of quirks (mostly around how it sort of weirdly conflates having one particular template with having a set of templates), but it's one of the safest template languages to just pick up and throw some stuff out; it's quite strong against injection attacks.

    To assist in using JSON, you'll want to declare a struct holding all your config, at which point it becomes quite surprisingly simple to load it from and save it to a file.

    Once you have that functionality, I suggest using it for your initial creation as well. I have a program here where I've been using the pattern that I create a default config sample within the program, and if I can't find the config you specify, I print out an error, and the sample JSON file that constitutes a full, legal config for the user's convenience. I wouldn't ship anything that I'd expect to end up in a Linux distro or something that way, but for an internal tool it seems a decent enough pattern.

    Finally, I'd suggest reddit.com/r/golang is a better sort of place for this sort of thing if you want a review, and there's probably even better places, as I'm not convinced that a karma-based voting site is all that great for reviews for beginners. (It's really easy for a post like that to pick up a couple of early downvotes, and consequently lose all visibility to the people who are willing to help.) But if you're going to use one, /r/golang would be better.

    • muzzammildotxyz 6 years ago

      Thanks for the suggestion :D

      I am not shipping it to any distro or something like that. I was using this for myself and just made it open source.

      Thank you for taking your time and reviewing this. As for /r/golang, I will look into it. :)

  • eptcyka 6 years ago

    It's GoLang, because that's easier to search for.

    • dewey 6 years ago

      I was just nitpicking, it's not really an important point.

      People usually prefer "Go" or "Golang", I only mentioned it because I noticed it in the project description.

  • muzzammildotxyz 6 years ago

    Thank you for taking your time to review my code. Yes, it is a beginner's project :D

    1) How do I format the JSON for readability or should I ignore that?

    2) Thanks for that. Didn't know about it. Any ideas on how I can fix that? Maybe I can check if it is a file and not a directory and ask for input again for y/n?

  • abiox 6 years ago

    i do agree that it makes sense to use the stdlib where possible.

    however i am a bit wary... my first "real" (but small) project in go was to process some xml. turns out the stdlib parser doesn't handle self-closing tags very well, and would simply not pick up some important content from files i was working with.

    i would wager the json stuff is probably a much hotter code path so it's probably fine.

badrabbit 6 years ago

A few concerns I have...

1) The logs are not sent encrypted, this exposes them for every smtp server and mitm party between unencrypted hops

2) you have to save a plain text version of your log mailer's smtp password to disk

3) Monitoring by email sucks and future compromise of any recipients' inbox exposes all historical logs

I have written something similar in the past as well and seen email monitoring in real world scenarios. You're not doing it "wrong" per se ,but I think modern protocols allow better solutions.

For instance,you can POST the logs to a server(ec2 instance or DO droplet) over TLS, have it generate a link and email that link. You can then control link expiry and encrypt the logs so that they are decrypted in-browser via Webcrypto. You can also do some sort of push monitoring(in addition to sending the link via email) by the server which lets you avoid the whole "email messages can be stuck on an smtp hop for 24-48 hours" and having to monitor for NDR's before resending the logs.

  • muzzammildotxyz 6 years ago

    Yes, these are valid concerns. 1) I don't know how to encrypt logs and send them. If you can help please open a PR on GitHub or walk me through it :)

    2) How else should I store it? Should I encrypt the json file with a master key?

    3) This is correct but I just wanted a quick way to do it :) Thanks for the concerns.

heegemcgee 6 years ago

Overall, i would say "using email for system tasks considered harmful". I have worked several devops / sysadmin jobs where my inbox took weeks to tame with filters because of rampant abuse of automated system emails.

Every alert should be actionable. And email doesn't have good reliability or timeliness - it can take hours for me to get a push notification on my phone that there is email, and in the evenings, i really shouldn't be looking at email at all. So we should be using a proper alert system via SMS (pagerduty is pretty great for this, but i also like twilio, and amazon SNS is just fine too).

More germaine to the topic at hand: I'd recommend a) setting up log monitors with Nagios, or Zabbix, or your favorite tool. You want to regex match on certain strings in the log file, like "Deadlock" or "out of memory". Pass that alert on to your monitoring system and get a proper, actionable alert.

And b), aggregating the logs. As far as convenient access, i'd recommend Graylog (or ELK or Splunk) if you have more than a handful of nodes. This makes it easy to search through logs or review them without signing into all those nodes. You can also push them over to Amazon Cloudwatch Logs for archival and rudimentary search.

  • AnIdiotOnTheNet 6 years ago

    > Every alert should be actionable.

    How do you know the difference between "everything is working properly" and "the logging and/or monitoring has stopped working"?

    • heegemcgee 6 years ago

      Who will watch the watchmen, right? :D It's a real concern. Personally, i have a monitoring agent running, and then i have the config management agent (puppet) validate that the monitoring agent is running.

      And what Dewey said is absolutely right - you can monitor the code / service itself through health checks. In the case of a reports service, perhaps your monitor asks the API for a very small report. Or you could implement a special endpoint / controller that calls on the core code. I recently implemented a monitor that emulates a typical user session, logging in, performing popular tasks, and logging out. If any step in that process has an error, i get an alert with the step listed, and i instantly have some idea of where things are jammed. In this manner, i don't need to have pre-defined log monitors for specific errors; i can catch novel error types by virtue of exercising the code and watching for the expected responses - 200 in the header, ability to perform tasks that are only available on login, checking for certain strings in the response, etc.

    • dewey 6 years ago

      You instrument your code instead of just logging. To see that your metric export is working you can regularly export a simple value and check for it’s existence.

  • muzzammildotxyz 6 years ago

    I just made it get the job done and email was the easiest way to do it. Also, I get emails pretty quickly, there isn't even a delay of 5 minutes.

    Thanks for the concern :)

cejast 6 years ago

I thought this was what logrotate was for?

https://linux.die.net/man/8/logrotate

  • submeta 6 years ago

    My thought also. - I was wondering why this post made it to the first page of HN. Obviously many fellow HN users find it valuable / do not know that there are solutions to this.

    • dvfjsdhgfv 6 years ago

      I'm baffled by this, too. Maybe it's interesting for people who are interested in learning Golang, but I think it's clear it's not the best piece of code to learn from.

      • muzzammildotxyz 6 years ago

        Gee, thanks for the boost in confidence. :|

        • dvfjsdhgfv 6 years ago

          I think you're self-confident enough.

dsr_ 6 years ago

It's very nice that you can specify the period between sending logs in nanoseconds, microseconds or milliseconds, but also utterly useless. Even if there's a time library being used that has these options, it's appropriate to only advertise the units that make sense.

Sadly, it does not include "days", although one can always specify the value in milliseconds. (The fifth hyperfactorial, as recently mentioned.)

  • muzzammildotxyz 6 years ago

    Well, you caught me. I copied and pasted that line from Go Source comments :D

    But hey, you can use 1h4ms6ns as intervals :D

    Maybe I should parse days... Thanks for the Idea.

noponpop 6 years ago

Have you considered syslog, which represents a standard for shipping logs?

hestefisk 6 years ago

You can get FreeBSD to email any log periodically using cron. No go program needed. This seems like an absolute overkill but it was probably fun writing the code.

  • muzzammildotxyz 6 years ago

    Yes, it was a great experience. Just made it to see if I could :)