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

feat(logging): added logging support #240

Merged
merged 5 commits into from
Nov 12, 2021
Merged

feat(logging): added logging support #240

merged 5 commits into from
Nov 12, 2021

Conversation

kpagacz
Copy link
Contributor

@kpagacz kpagacz commented Nov 9, 2021

Take out any module for a ride to test.

IMPORTANT: this does not add logging changes in the shiny input because loggers function does not work with custom namespaces. Created an issue to add it later #239

* added support for `logger`
* now able to log to `teal.modules.general` namespace using the logger
package
* also added info level logs to the modules
Closes #224
@kpagacz kpagacz added the core label Nov 9, 2021
@kpagacz kpagacz requested a review from a team as a code owner November 9, 2021 16:20
@insights-engineering-bot
Copy link
Contributor

insights-engineering-bot commented Nov 9, 2021

File Coverage
All files 2%
R/tm_a_pca.R 0%
R/tm_a_regression.R 0%
R/tm_data_table.R 0%
R/tm_file_viewer.R 0%
R/tm_g_association.R 0%
R/tm_g_bivariate.R 26%
R/tm_g_distribution.R 0%
R/tm_g_response.R 0%
R/tm_g_scatterplot.R 0%
R/tm_g_scatterplotmatrix.R 7%
R/tm_missing_data.R 0%
R/tm_outliers.R 0%
R/tm_t_crosstable.R 0%
R/tm_variable_browser.R 0%
R/utils.R 0%
R/zzz.R 0%

Minimum allowed coverage is 80%

Generated by 🐒 cobertura-action against ec9984a

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2021

Unit Tests Summary

    1 files      4 suites   26s ⏱️
135 tests 135 ✔️ 0 💤 0
168 runs  168 ✔️ 0 💤 0

Results for commit ec9984a.

♻️ This comment has been updated with latest results.

@Polkas Polkas self-assigned this Nov 10, 2021
@Polkas
Copy link
Contributor

Polkas commented Nov 10, 2021

I think each log_info should have argument namespace = "teal.module.general". The default one is a "global" one, when we use many packages at once we could not recognise the source.
I think we should think of namespace usage in teal.log_layout too.

@kpagacz
Copy link
Contributor Author

kpagacz commented Nov 10, 2021

I think each log_info should have argument namespace = "teal.module.general". The default one is a "global" one, when we use many packages at once we could not recognise the source. I think we should think of namespace usage in teal.log_layout too.

The default behaviour is to log to the namespace equal to the package name.

@gogonzo gogonzo self-assigned this Nov 10, 2021
Copy link
Contributor

@Polkas Polkas left a comment

Choose a reason for hiding this comment

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

Please update the DESCRIPTION file to include logger and we are free to go.

R/tm_a_pca.R Outdated Show resolved Hide resolved
Copy link
Contributor

@Polkas Polkas left a comment

Choose a reason for hiding this comment

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

In my opinion we should merge it and later add shiny input logger separately when it will be available (new issue). @gogonzo are you content?

@gogonzo
Copy link
Contributor

gogonzo commented Nov 10, 2021

I wonder why not to add logger::log_info("Initialized tm_x_xxx") at the end of the srv_x_xxx module.

  • Also, add logging changes in user input at the trace level.
    I understand this needs to wait for some logger improvement - according to your explaination on yesterdays daily

@kpagacz
Copy link
Contributor Author

kpagacz commented Nov 10, 2021

Because it seems like it tries to be a trace log and not an info log, by default, we only want to show the most critical information, and IMO this is not it. You are welcome to create a discussion task where we can discuss if we're going to add more detailed logs to modules, but my knee-jerk reaction is to log less, not more, unless we have a specific reason to do it.

Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

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

👍

@kpagacz kpagacz merged commit bfb3919 into main Nov 12, 2021
@kpagacz kpagacz deleted the 224_add_logger@main branch November 12, 2021 09:39
@gogonzo gogonzo self-assigned this May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initialize the logger in .onLoad and add a logging info entry to each module
4 participants