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

Allow custom formats for the Access logs; like JSON. #62

Open
robertmassaioli opened this issue Dec 16, 2014 · 15 comments
Open

Allow custom formats for the Access logs; like JSON. #62

robertmassaioli opened this issue Dec 16, 2014 · 15 comments
Labels

Comments

@robertmassaioli
Copy link

I have been reading through the code and I am pretty sure that I need to override the logA method in order to get custom logging support. I asked a question on SO about it just in case.

But I don't think that I can currently override this logA functionality without changing the code inside of Snap Server. If this is correct then, for this issue, it would be great if I could inject a method into the snap server configuration that would intercept the access log generation and let me write my own access log lines in a custom format.

@gregorycollins
Copy link
Member

Correct, although you can retarget the logs (see setAccessLog), currently we only log in combined format and this is not configurable.

@robertmassaioli
Copy link
Author

Thank you, this is good to know. Is it a feature that you would be interested in having? Aka, if somebody wrote a pull request for this would the Snap team accept it?

@gregorycollins
Copy link
Member

Yes. The broader issue of how you configure the HTTP server, though, is something that I want to make extensive changes to for 1.0, so I would worry that a patch against the current implementation would be wasted work. In the new server you will maybe just provide an OutputStream LogEntry to the server for the error logging.

@robertmassaioli
Copy link
Author

Okay, cool, In that case I might wait for the release of Snap 1.0. However, how far away is that? Is there a page (or something) that I could follow to track when 1.0 was released?

@robertmassaioli
Copy link
Author

@gregorycollins I'm currently working on this code in a branch because I really really want and need this functionality because it would allow me to output all of my logs in JSON format. If I were to write this code such that it could be merged into both 0.9-stable and master would you accept both PR's?

You can see my initial attempts here: 0.9-stable...robertmassaioli:issue/62-custom-access-and-error-log-handlers

@robertmassaioli
Copy link
Author

Please have a look at my initial attempts and let me know if I am going in the right direction. I'd like feedback now before I spin my wheels too fast in one direction. 😃

@robertmassaioli
Copy link
Author

I have tested this branch against my Snap application My Reminders and it is working:

My Reminders JSON log

So my branch seems to be workable to accomplish the goal of logging your access and error logs in a custom format.

You can see an example of me using this branch in the My Reminders code here: https://bitbucket.org/atlassianlabs/my-reminders/branch/issue/MR-7-json-logs-for-my-reminders#diff

@robertmassaioli
Copy link
Author

Ping. These PR's have been open for a week now without comment. Sorry that I'm pinging so soon but:

  1. I suspect that there may still be more work to do on these PR's; especially if you want me to use the OutputStream code.
  2. The sooner this code gets released the sooner that I can hookup better Logstash / Kibana logging to my services. Which I think they desperately need.

Cheers and sorry if I'm commenting too frequently! Just tell me to stop. 😄 I won't mind!

@gregorycollins
Copy link
Member

Ping. These PR's have been open for a week now without comment.

I was on vacation all last week and didn't get to it on the weekend because I have a houseguest. I'll take a look today.

@robertmassaioli
Copy link
Author

Wow, I am such a pest. I am really sorry. Thank you so much for looking at this so soon. In my defence I guess I'm just excited about submitting changes back to snap.

@gregorycollins
Copy link
Member

Thank you so much for looking at this so soon. In my defence I guess I'm just excited about submitting changes back to snap.

Believe me, I'm quite happy you sent the patches :)

@robertmassaioli
Copy link
Author

@gregorycollins What do you think about this snippet for the release logs:

In the latest release of snap-server we include two new configuration options aimed at letting you
control your log file output format. The two functions are setAccessLogHandler and
setErrorLogHandler. These methods respectively will let you control how the access and error log
lines are rendered respectively. This is useful if you wist to modify the log line output; for
example, if you wish to log in a custom data format (like JSON) then now you can. For more information please read the docs.

What do you think? I was trying for short and to the point.

@robertmassaioli
Copy link
Author

@gregorycollins Also, I have bumped the snap-server version to 0.9.6 because I have only made additive changes. Not sure if anybody else has made breaking changes that would require a bump to 0.10.0? At any rate, you can see that my additions to 0.9-stable still build against snap 0.14-stable: https://bitbucket.org/snippets/robertmassaioli/rkpK7

Cheers!

@gregorycollins
Copy link
Member

See my comment on the changeset, we will need a major bump for this.

Your initial wording sounds OK to me, but the best way to handle this will be to open a pull request on snap-website, we can discuss the minutae there.

@robertmassaioli
Copy link
Author

I have responded: https://github.com/snapframework/snap-server/pull/71/files#r31794779

I hope that changing the Config instance is not enough to prevent a backport. After all, it implements Monoid and even the docs recommend to create it with the monoid instance.

Edit: sorry that it has taken me so long to get back to you. I just had to compete in and run in a big hackathon at work: https://www.atlassian.com/company/about/shipit I mention it because there were two (maybe three) projects in that Hackathon that were using snap. Win! Thought you might appreciate some more examples of snap used in the wild.

robertmassaioli added a commit to robertmassaioli/snap-server that referenced this issue Jun 11, 2015
robertmassaioli added a commit to robertmassaioli/snap-server that referenced this issue Jun 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants