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

Improve chug performance (and clean up the code while we're at it). #33

Closed
goonzoid opened this issue May 7, 2018 · 4 comments
Closed
Labels

Comments

@goonzoid
Copy link
Contributor

goonzoid commented May 7, 2018

While doing this Diego story to make Chug work with the new "pretty" log format, I noticed that the API we had introduced when adding the so called "pretty" log format was not the best, and it was making it hard to make Chug generic.

Specifically, having multiple ways to represent a log entry in memory seemed wrong. IMHO, we only need one way to represent a log in memory, and then we can manipulate the information according to the type of JSON we're trying to generate and write later on. I've taken some steps towards that, and the PrettyFormat, RFC3339Time types that were recently introduced are now either gone, or no longer exported. Howerver, in order to get the story done in a reasonable time, we've introduced a pretty ugly bit of code which tries to unmarshal one format, and then if it fails, creates another decoder and tries to unmarshal again. Obviously this is suboptimal, and could get a bit slow when chugging large volumes of logs.

I think there are two ways we could go from here:

  • Continue to decouple the types in package chug from those in the core lager package. We should be able to introduce a single type that we can unmarshal all sorts of log formats in to (i think)
  • Make breaking changes to the lager.LogFormat type such that it can be used in all cases inside package chug. This breaks API that was supported in the last tagged version, so would need a little coordination with all consumers, but may be the better solution.

When I have a bit more time, I may circle back with specific code examples, etc. Very open to ideas and thoughts on this.

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/157365724

The labels on this github issue will be updated when the story is started.

@sunjayBhatia
Copy link
Contributor

closing this, it involves breaking changes which we don't want to introduce at this time

@sunjayBhatia
Copy link
Contributor

if there are concerns about lager perf. + measurements, we can reconsider

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/169630810

The labels on this github issue will be updated when the story is started.

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

3 participants