Eliminate human log format #567

Closed
wants to merge 35 commits into
from

Conversation

Projects
None yet
5 participants
@htgoebel
Contributor

htgoebel commented Sep 2, 2012

The implementation was not correct: it changes the log-format for all
loggers. Doing it right is not worth the effort, esp. since local
log-files are using ISO timestamps meanwhile.

Please apply this after `use-standard-logging.module.

htgoebel added some commits Aug 22, 2012

Do not use a global `logger` in test_logging.
This is a preparation for switching to the new logging module. This
may not be perfect, since there are still global variables in shinken.log,
but all test-cases still pass.
New function `send_result()` to avoid misusing loggers for this.
Currently `send_result()` is only a wrapper to console_logger.info(),
but this will be changed as soon as the standard-log-system is used.
Remove test-cases no longer valid.
The interface will change:
- no more global variable `human_format`
- Log.log() require to level to be passed and not be used in shinken.
Base `shinken.log.Log` on std-Python `logging.Logger`.
Still to do: send broks, local logging, human-format.
Make `Log.set_level()` and alias for std `setLevel()`.
Todo: Remove this alias when cleaning up.
Add test-case for console_logger.
This one is writing messages to the console, broker and (if applicable)
to the local log-file.
Fix log-level when logging config-errors in business-rules.
This is required to make test_business_correlator.py pass.
Eliminate `human_timestamp_log`.
The implementation was not correct: it changes the log-format for all
loggers. Doing it right is not worth the effort, esp. since local
log-files are using ISO timestamps meanwhile.
@xkilian

This comment has been minimized.

Show comment
Hide comment
@xkilian

xkilian Sep 3, 2012

Contributor

How much risk is there to integrating this patch. Should we wait after Shinken 1.2.1? (first bug fixe release of 1.2)

I will test this on tuesday.

Contributor

xkilian commented Sep 3, 2012

How much risk is there to integrating this patch. Should we wait after Shinken 1.2.1? (first bug fixe release of 1.2)

I will test this on tuesday.

@naparuba

This comment has been minimized.

Show comment
Hide comment
@naparuba

naparuba Sep 4, 2012

Owner

Yes, we will need a 1.2.1 for the Graphite bug at least (next monday?), so
after this version, we can get the whole logging pack :)

Jean

On Mon, Sep 3, 2012 at 3:51 PM, xkilian notifications@github.com wrote:

How much risk is there to integrating this patch. Should we wait after
Shinken 1.2.1? (first bug fixe release of 1.2)

I will test this on tuesday.


Reply to this email directly or view it on GitHubhttps://github.com/naparuba/shinken/pull/567#issuecomment-8238962.

Owner

naparuba commented Sep 4, 2012

Yes, we will need a 1.2.1 for the Graphite bug at least (next monday?), so
after this version, we can get the whole logging pack :)

Jean

On Mon, Sep 3, 2012 at 3:51 PM, xkilian notifications@github.com wrote:

How much risk is there to integrating this patch. Should we wait after
Shinken 1.2.1? (first bug fixe release of 1.2)

I will test this on tuesday.


Reply to this email directly or view it on GitHubhttps://github.com/naparuba/shinken/pull/567#issuecomment-8238962.

@xkilian

This comment has been minimized.

Show comment
Hide comment
@xkilian

xkilian Sep 5, 2012

Contributor

Well, we should aim to have everything ready by friday. So come monday morning, new users can download 1.2.1 and get cooking. After that 1.2.2 for whatever comes out of next week or the two weeks after that…

Which is why we need to have the discussion about stable versus dev.

Contributor

xkilian commented Sep 5, 2012

Well, we should aim to have everything ready by friday. So come monday morning, new users can download 1.2.1 and get cooking. After that 1.2.2 for whatever comes out of next week or the two weeks after that…

Which is why we need to have the discussion about stable versus dev.

@htgoebel

This comment has been minimized.

Show comment
Hide comment
@htgoebel

htgoebel Sep 5, 2012

Contributor

Argl! This discussion as nothing to do with this pull-request. It should not be discussed as a side-note to some pull-request, but on the mailing-list.

Contributor

htgoebel commented Sep 5, 2012

Argl! This discussion as nothing to do with this pull-request. It should not be discussed as a side-note to some pull-request, but on the mailing-list.

@xkilian

This comment has been minimized.

Show comment
Hide comment
@xkilian

xkilian Sep 6, 2012

Contributor

Yes, of course, I tend to go off on tangents…

I for one, would like to see better logging for 1.2.x. But I am a little weary at breaking more things that are not covered by the test cases. But supporting Shinken with the current logs is currently sub-optimal.
Hartmut, can this series of pull requests be put in fairly safely. I think we have a good grasp on the logging issues that can happen.

What do you think? Your changes would handle issue #583. I went through over 3/4s of your change request and I agree with most everything in there.

Contributor

xkilian commented Sep 6, 2012

Yes, of course, I tend to go off on tangents…

I for one, would like to see better logging for 1.2.x. But I am a little weary at breaking more things that are not covered by the test cases. But supporting Shinken with the current logs is currently sub-optimal.
Hartmut, can this series of pull requests be put in fairly safely. I think we have a good grasp on the logging issues that can happen.

What do you think? Your changes would handle issue #583. I went through over 3/4s of your change request and I agree with most everything in there.

@htgoebel

This comment has been minimized.

Show comment
Hide comment
@htgoebel

htgoebel Sep 6, 2012

Contributor

@xkilian: Please mind: This pull request is for eliminating human-log-format. It is based on pull #566.

For fixing issue #583, I would not apply this pull-request (nor #566) as it also introduces other changes.
You may want to cherry-pick b87bc02, 95c65bf and 6e63081.

Contributor

htgoebel commented Sep 6, 2012

@xkilian: Please mind: This pull request is for eliminating human-log-format. It is based on pull #566.

For fixing issue #583, I would not apply this pull-request (nor #566) as it also introduces other changes.
You may want to cherry-pick b87bc02, 95c65bf and 6e63081.

@xkilian

This comment has been minimized.

Show comment
Hide comment
@xkilian

xkilian Sep 6, 2012

Contributor

Tried cherry picking the 4-5 commits that seemed to be required, but I can'tget it to be clean and functional.

I merged both your pull requests related to logging and tested using that. Looks good.

One question: Does the brokhandler call at the bottom of log.py need a try catch statement, as _broker.add is a remote method. I was not sure if it should be a _brokhandler.handleError?

Contributor

xkilian commented Sep 6, 2012

Tried cherry picking the 4-5 commits that seemed to be required, but I can'tget it to be clean and functional.

I merged both your pull requests related to logging and tested using that. Looks good.

One question: Does the brokhandler call at the bottom of log.py need a try catch statement, as _broker.add is a remote method. I was not sure if it should be a _brokhandler.handleError?

@xkilian

This comment has been minimized.

Show comment
Hide comment
@xkilian

xkilian Sep 6, 2012

Contributor

I can issue a new pull request with the small corrections needed to merge into 1.2.

--- edit ---
Argh.. me and git had a problem. I suggest you correct the file that prevents auto-merge, add the missing try (if applicable) and I vote for a merge for 1.2.1.

Contributor

xkilian commented Sep 6, 2012

I can issue a new pull request with the small corrections needed to merge into 1.2.

--- edit ---
Argh.. me and git had a problem. I suggest you correct the file that prevents auto-merge, add the missing try (if applicable) and I vote for a merge for 1.2.1.

@xkilian

This comment has been minimized.

Show comment
Hide comment
@xkilian

xkilian Sep 18, 2012

Contributor

I merged the changes back into the latest Shinken master and logging doesn't seem to be working correctly. All the daemon log files are empty with only a single line logged. Merged std-logging + no human logging.

Contributor

xkilian commented Sep 18, 2012

I merged the changes back into the latest Shinken master and logging doesn't seem to be working correctly. All the daemon log files are empty with only a single line logged. Merged std-logging + no human logging.

@naparuba

This comment has been minimized.

Show comment
Hide comment
@naparuba

naparuba Jan 15, 2013

Owner

Hi,

what is the state of this ticket? I'm ok for removing the human_format thing from conf and code. Was an error to add it.

Owner

naparuba commented Jan 15, 2013

Hi,

what is the state of this ticket? I'm ok for removing the human_format thing from conf and code. Was an error to add it.

@Frescha

This comment has been minimized.

Show comment
Hide comment
@Frescha

Frescha Mar 1, 2013

Collaborator

Any updates?

Collaborator

Frescha commented Mar 1, 2013

Any updates?

@naparuba naparuba added the Bug label Mar 14, 2014

@Seb-Solon Seb-Solon referenced this pull request Apr 21, 2014

Closed

Shinken 2.2 Roadmap #1175

6 of 6 tasks complete

@Seb-Solon Seb-Solon self-assigned this May 15, 2014

@Seb-Solon Seb-Solon modified the milestones: 2.2 (Maleable Muskrat) **doc, cli & arbiters**, 2.0 (Kooky Katydid) **flexibility** May 15, 2014

@Seb-Solon

This comment has been minimized.

Show comment
Hide comment
@Seb-Solon

Seb-Solon Jun 5, 2014

Contributor

Merged with #1201. Thanks a lot @htgoebel 🍻

Contributor

Seb-Solon commented Jun 5, 2014

Merged with #1201. Thanks a lot @htgoebel 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment