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 HTTP logging to be controlled #2246

Closed
wants to merge 7 commits into from
Closed

Conversation

otoolep
Copy link
Contributor

@otoolep otoolep commented Apr 11, 2015

This change also re-enables DQ testing, as well as remove a bogus warn statement.

@otoolep otoolep changed the title Increase Circle testing timeout Allow HTTP logging to be controlled Apr 11, 2015
@otoolep
Copy link
Contributor Author

otoolep commented Apr 11, 2015

@corylanou @jwilder

Logger *log.Logger
WriteTrace bool // Detailed logging of write path
Logger *log.Logger
HTTPAccessLogging bool // Log every HTTP access.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just make this something like EnableLogging or LoggingEnabled? This name is confusing when I don't see it in context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

@otoolep
Copy link
Contributor Author

otoolep commented Apr 11, 2015

I also increased the interval between querying the server during testing to 100ms. I originally suggested 10ms, may mean less strain on the Circle container, and it should not actually make a difference to how long tests take to run.

@@ -239,6 +240,10 @@ func NewConfig() *Config {
c.Snapshot.BindAddress = DefaultSnapshotBindAddress
c.Snapshot.Port = DefaultSnapshotPort

c.Logging.HTTPAccess = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an HTTPAPI section. Should we put logging under that? This logging section is starting to feel out of place. Each are could have it's own logging flag now if we wanted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly. I could go either way. Would you agree to punting on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Distinct PR, perhaps?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree w/ @corylanou.

@otoolep
Copy link
Contributor Author

otoolep commented Apr 11, 2015

Integration test output is so much easier to work with now, I do think we should merge this (or something close to it).

@otoolep
Copy link
Contributor Author

otoolep commented Apr 11, 2015

Updated with change to HTTP logging flag.

@corylanou
Copy link
Contributor

@otoolep I'm fine with it in current state. Our config might need some re-org at some point in the future anyway.

I do agree this was very much needed. It was getting impossible to look through the failed CI output. Thanks!

@otoolep
Copy link
Contributor Author

otoolep commented Apr 11, 2015

Also fixes issue #2245.

@otoolep
Copy link
Contributor Author

otoolep commented Apr 11, 2015

Thanks @corylanou -- CircleCI runs already looking more stable on this branch.

It would appear this should not have been committed.
Each of these tests relies on a write of only a few points. It simply
should not take 60 seconds.
@otoolep
Copy link
Contributor Author

otoolep commented Apr 16, 2015

Fixes issue #2302

@otoolep otoolep force-pushed the increase_test_timeout branch 2 times, most recently from abbf99b to ef413bd Compare April 17, 2015 06:03
@otoolep
Copy link
Contributor Author

otoolep commented Apr 17, 2015

Closed in favour of #2320

@otoolep otoolep closed this Apr 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants