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

Switch from lager to hut-based logging #66

Merged
merged 3 commits into from
Feb 11, 2016
Merged

Switch from lager to hut-based logging #66

merged 3 commits into from
Feb 11, 2016

Conversation

tolbrino
Copy link
Contributor

As part of the switch the exometer_report_lager reporter will be moved
into its own repository.

In case we agree to switch I will finalize the move of the reporter and clean up the docs as well.

As part of the switch the `exometer_report_lager` reporter will be moved
into its own repository.
@uwiger
Copy link
Member

uwiger commented Jan 20, 2016

I agree that insofar as the exometer_report_lager module should exist at all, it should be in exometer_core. Personally, I think it was a mistake.

However, using lager 3.x, one could modify it to use a separate sink, and have the metrics output to a separate log. I would much prefer that.

@tolbrino
Copy link
Contributor Author

Did you mean should not be in exometer_core? If not I'm a little confused :-)

@uwiger
Copy link
Member

uwiger commented Jan 20, 2016

I mean that exometer_report_lager didn't turn out as useful as I'd hoped, but it belongs in exometer_core, not exometer.

OTOH, if we are now removing the lager dependency from exometer_core, perhaps it should not be in there? :)

@tolbrino
Copy link
Contributor Author

I updated the PR to use the release of hut. Once this is merged, I can update the exometer_lager repo.

@tolbrino
Copy link
Contributor Author

tolbrino commented Feb 4, 2016

@uwiger Complaints?

@uwiger
Copy link
Member

uwiger commented Feb 4, 2016

@tolbrino am I correct in concluding that the default behavior for ?log(debug, ...) is to call the error_logger?

I think the appropriate default action for debug logging would be to do nothing, unless debug is enabled.

@tolbrino
Copy link
Contributor Author

tolbrino commented Feb 5, 2016

@uwiger You are correct. So far I assumed the log level checks would be the responsibility of the underlying log system. error_logger never supported such thing, lager however does.

So you'd be fine with having a simple debug toggle to enable debug logging while all other levels are passed through directly?

@uwiger
Copy link
Member

uwiger commented Feb 5, 2016

@tolbrino I would. The lager transform inserts a check using whereis/1 ...

@tolbrino
Copy link
Contributor Author

@uwiger I've added something to enable debug logging to the latest https://github.com/tolbrino/hut master.

@uwiger
Copy link
Member

uwiger commented Feb 10, 2016

Looks ok to me

@tolbrino
Copy link
Contributor Author

I updated hut to the latest version. Feel free to merge.

@uwiger
Copy link
Member

uwiger commented Feb 11, 2016

Shouldn't there be some mention in the documentation?

For those who used lager before and depended on exometer to pull it in (hopefully not too many), this change will require them to state the lager dependency explicitly. At the very least, they will need to insert a config to keep the lager logging behavior.

@tolbrino
Copy link
Contributor Author

I think that kind of info would be better suited for the release notes of the next release.

uwiger added a commit that referenced this pull request Feb 11, 2016
Switch from lager to hut-based logging
@uwiger uwiger merged commit 6bdd642 into master Feb 11, 2016
ldr added a commit to ldr/netlink that referenced this pull request Jul 1, 2017
Made same changes as were done for feuerlabs/exometer_core

See related discussions:

* Feuerlabs/exometer_core#57
* Feuerlabs/exometer_core#66
@tolbrino tolbrino deleted the tb/hut branch April 9, 2018 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants