Charm Needed: Papertrailapp

[Marco Ceppi]

Papertrail app is Hosted log management for servers, apps, and cloud services.

[Jorge Castro (jorge)]

Hey Marco is this one ready for review at some point?

[Charles Butler (lazypower)]

Initial charm complete and ready for charm review with rsyslog support, monitoring all logs in /var/log, and arbitrary application logs.

Many thanks go to Marco Ceppi for assisting in the approach of this charm.

[Charles Butler (lazypower)]

Additionally, I have an open support ticket into the PaperTrail team to address the lack of an Icon for the charm.

Without branding guidelines I hesitate to arbitrarily guess at their style format and preferred logo resource.

You can review the open support ticket here:
http://help.papertrailapp.com/discussions/questions/7248-papertrail-icon

[Marco Ceppi (marcoceppi)]

 Hi Charles! Thanks for taking the time to submit this charm to the charm store. We really appreciate the effort so far. I've included a first review of the charm below!

# Proof

I ran `juju charm proof` from the charm-tools package with the following results:

W: No icon.svg file.

Typically, we'd like an icon included. However, it appears your working with upstream so this can be omitted for the time being. If response from them is sluggish, you could consider just having a blue icon to match their branding color and a while "pt" since they don't seem to have a logo. The icon can always be updated post-promulgation.

# Review

I don't like that the install hook exits > 0 if the user hasn't set the port. Typically, unless someone has seeded the charm config via the --config flag, it hasn't been set yet. In my opinion it should just exit 0 there, and wait for the user to set it via config-changed.

There's no need to have `juju-info-relation-joined` and `juju-info-relation-changed` hooks. You can omit them since `hooks/install` is guaranteed to run already. Juju will assume a hook exits cleanly if it doesn't exist.

The README is a bit lacking, while it covers a few good points, we'd prefer to see the configuration section more drawn out rather than a copy/paste of the yaml file. Ideally you'd cover each option, describe what it does, examples of values to set the key to, and links to any additional resources on how to use that option. Also, while you show how to deploy and set the config, you should include deploying another service and the add-relation as not everyone will know that it needs to be attached in order for the subordinate to work.

I don't understand the `juju-info-relation-departed` hook, why are you searching rsyslog.conf if all the charm touches is `/etc/rsyslog.d/25...conf`

Your maintainer email, while fully qualified, is not a routeable address.

The provides block is confusing, it provides a logging relation and defines a new interface, but has no hooks for that relationship. I'm not sure if the papertrail charm providing anything makes sense.

Is the gem the only way to add non-rsyslog logging methods? I'm curious if there is a tar.gz or other method of installation outside of gem.

# Knitpicks

It might instead be wiser to move the majority of the install code (creating rsyslog files, reading configuration data, etc) to the config-changed hook. That way, everytime a user changes a setting, like the port, the config-changed hook will be executed and the code is updated.

I strongly recommend you add at least `set -e` to the top of your hooks, and consider making it `set -eux`. While the -e will exit the script if any of the commands exit > 0, the ux will dump xtrace and more information to the juju-log which might be helpful during debug

# Conclusion

All in all this is a great start to a charm, and baring a few adjustments and clarifications I think it'll be ready for the store soon. Thank you again for taking the time to work on this. I've moved the bug to Incomplete, when you're ready for another review please move the bug status to either "New" or "Fix Committed" for it to be automatically added back in to the rev...

[Charles Butler (lazypower)]

Marco,

I would like to thank you for your detailed review of my charm. I hastily put together the proof of concept which I'm certain
showed.

Charmers,

I've considered all of the feedback and placed this charm back into the review queue. I would like to call out a few things that have changed since the last revision provided:

I've cleaned up the README and expanded the missing/sparse sections. I've also added support for a secondary syslog daemon, syslog-ng.

A few things I'm still brain bending is the remote_syslog gem is the listed de-facto default for shipping arbitrary logs to papertrail. Upstream has alluded to a new client in the works that is not part of the ruby ecosystem - but its not released for public consumption yet.

An alternative solution I've come up with is [nxlog-ce](http://nxlog-ce.sourceforge.net/). Its light weight and doesn't come with the ruby dependency chain, however its not in the 12.04 repositories. This means I'll probably have to create a PPA to house the packages which will be a fun new adventure.

I've established a good rapport with Leon and Troy over at Papertrail support while building this charm and have spoken to them at length of terminology used in the documentation, and clarity / ease of setup. They were happy to see the change from a port configuration to a one liner copy/paste setup.

I look forward to the next step in the review.

Thanks!

[Jorge Castro (jorge)]

Hi Charles, thanks for rocking this:

If you want to do nxlog-ce I think it's probably best to split that off in another charm with a relationship with this one. As a first cut I wouldn't worry about packaging it, just get it working and we can go from there.

However if papertrail recommends today is to do the remote syslog gem I am wondering if we can just roll with that for now, document it in the readme, and go from there? It's not ideal but it gets papertrail in hands of users quicker so we can iterate.

Thoughts?

[Charles Butler (lazypower)]

Jorge, I agree with your comment of using the remote_syslog gem. This is defacto the quickest way to get integrated. There are other means through the use of rsyslog, however it makes for a more difficult configuration by the end user and does not aggregate upstream as nicely as the remote_syslog gem provides.

I'm more than happy to provide a second iteration that is completely absent of the ruby components.

I'm going to re-open this charm for review since I've placed some new components in the charm packing like the revised icon, and readme updates.

[Marco Ceppi (marcoceppi)]

Hi Charles,

Thanks for your updates to this charm during the review process! I've taken another look at the charm and have the following review results:

# Proof

There were several changes to the proofing rules in the past few weeks, here are the updated results of `juju charm proof` from the charm-tools package:

E: config.yaml: type of option applicationlogs is specified as string, but the type of the default value is NoneType
E: config.yaml: type of option syslog_endpoint is specified as string, but the type of the default value is NoneType

This is a parsing concern with the config.yaml, you provide options with an explicit type. For sake of parsing, assign "null" items as empty strings `""` instead of null to avoid this NoneType error. You'll also need to update the logic in your code from `null` to ""

# Review

You don't have the proper icon formatting for your icon.svg file, please see https://juju.ubuntu.com/docs/authors-charm-icon.html for a template an instructions on editing.

# Knitpicks

This could be a larger issue, however, you never remove the conf files for the various logging daemons during stop. Since stop hook is only called during unit teardown (and since you can't just `service rsyslog stop` since juju uses it), it might be worth deleting and cleaning up those 25-papertrail.conf files before restarting rsyslog. Otherwise the unit will continue to log even after the subordinate has been removed.

# Conclusion

You're one step closer to making it in to the charm store, baring a few adjustments and clarifications I think it'll be ready for the store soon. Thank you again for taking the time to work on this. I've moved the bug to Incomplete, when you're ready for another review please move the bug status to either "New" or "Fix Committed" for it to be automatically added back in to the review queue.

If you have any questions, comments, concerns, feel free to reply back to this bug, reach out to us in #juju on freenode.net, ask a question tagged "juju" on http://askubuntu.com, or mail our mailing list: <email address hidden>. Thanks!

[Charles Butler (lazypower)]

Thank you for the detailed review.

I've modified the stop script to remove the configuration, revised the icon.svg using the baseline juju charm svg template, and changed the invalid nulls to emptystring.

I feel this is ready for review again

[Marco Ceppi (marcoceppi)]

LGTM +1

Some minor pointers, the indents and formatting are kind of all over the place, for maintainability it'd be wise to align them all, otherwise thanks for the submission!