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

Bug 1812738 - allow user to set Glean log level #2459

Merged
merged 1 commit into from
May 5, 2023

Conversation

rosahbruno
Copy link
Contributor

@rosahbruno rosahbruno commented May 1, 2023

Allow the ability to configure the logging levels of Glean.

@rosahbruno rosahbruno force-pushed the 1812738-turn-off-logging branch 3 times, most recently from 5c9bef4 to 203a75c Compare May 1, 2023 19:38
@rosahbruno
Copy link
Contributor Author

@travis79
What are your thoughts on how I did this?

I set this up so that you can just call the function with your log preference. The other debug options all go through debug.rs and work a little differently since they rely on knowing if Glean is initialized or not. Since this is just a setting for the crate itself, it doesn't really care what is going on with Glean. I can update this to work similarly to the other debug options if we want this to feel the same.

@rosahbruno rosahbruno requested a review from travis79 May 1, 2023 19:46
@rosahbruno rosahbruno force-pushed the 1812738-turn-off-logging branch 2 times, most recently from 924f07d to ef898a1 Compare May 1, 2023 20:24
@travis79
Copy link
Member

travis79 commented May 1, 2023

I think this should be fine. I actually envisioned this added as a parameter to the configuration passed into Glean.initialize, that way it's not adding the burden of calling an additional API to set the logging level

glean-core/src/glean.udl Outdated Show resolved Hide resolved
@badboy
Copy link
Member

badboy commented May 2, 2023

I think this should be fine. I actually envisioned this added as a parameter to the configuration passed into Glean.initialize, that way it's not adding the burden of calling an additional API to set the logging level

Note that on Python we already pass in the log level, see https://mozilla.github.io/glean/book/user/debugging/python.html#logging-pings
We should stay consistent then and have it the same across languages (due to how things work the Python bindings configure the log level on the Python logger instance, not on the Rust side)

@travis79
Copy link
Member

travis79 commented May 2, 2023

I think this should be fine. I actually envisioned this added as a parameter to the configuration passed into Glean.initialize, that way it's not adding the burden of calling an additional API to set the logging level

Note that on Python we already pass in the log level, see mozilla.github.io/glean/book/user/debugging/python.html#logging-pings We should stay consistent then and have it the same across languages (due to how things work the Python bindings configure the log level on the Python logger instance, not on the Rust side)

Ahh, I'd forgotten that we had this in Python 🤦. Yes, let's go with consistency!

@rosahbruno rosahbruno force-pushed the 1812738-turn-off-logging branch 6 times, most recently from c69b3f7 to 2cd0428 Compare May 2, 2023 22:36
@rosahbruno rosahbruno marked this pull request as ready for review May 3, 2023 11:06
@rosahbruno rosahbruno requested a review from a team as a code owner May 3, 2023 11:06
@rosahbruno rosahbruno requested a review from badboy May 3, 2023 11:06
@rosahbruno
Copy link
Contributor Author

@badboy @travis79
Updated based on the feedback

glean-core/src/glean.udl Outdated Show resolved Hide resolved
glean-core/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

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

Sorry for one more round of sending this back.

glean-core/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@travis79 travis79 left a comment

Choose a reason for hiding this comment

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

I lean in @badboy's direction here on the default level of the logging, but no need to block on further review, I think.

docs/user/reference/general/initializing.md Outdated Show resolved Hide resolved
@rosahbruno rosahbruno force-pushed the 1812738-turn-off-logging branch 2 times, most recently from 3085ba8 to 37c48d7 Compare May 4, 2023 20:11
@rosahbruno rosahbruno force-pushed the 1812738-turn-off-logging branch from 37c48d7 to c852fe4 Compare May 4, 2023 20:17
@rosahbruno rosahbruno requested a review from badboy May 4, 2023 20:30
@rosahbruno
Copy link
Contributor Author

@badboy
I've addressed the comments you left. Now the logging level is only overwritten if provided by the user, otherwise it uses what we set elsewhere. I also updated the comments to call out "internal logging" rather than the rust crate.

@rosahbruno rosahbruno merged commit e541f3a into mozilla:main May 5, 2023
@rosahbruno rosahbruno deleted the 1812738-turn-off-logging branch May 5, 2023 10:48
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.

3 participants