-
Notifications
You must be signed in to change notification settings - Fork 86
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
Comments
Correct, although you can retarget the logs (see setAccessLog), currently we only log in combined format and this is not configurable. |
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? |
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 |
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? |
@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 |
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. 😃 |
I have tested this branch against my Snap application My Reminders and it is working: 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 |
Ping. These PR's have been open for a week now without comment. Sorry that I'm pinging so soon but:
Cheers and sorry if I'm commenting too frequently! Just tell me to stop. 😄 I won't mind! |
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. |
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. |
Believe me, I'm quite happy you sent the patches :) |
@gregorycollins What do you think about this snippet for the release logs:
What do you think? I was trying for short and to the point. |
@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! |
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. |
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. |
As requested.
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.
The text was updated successfully, but these errors were encountered: