Skip to content

Conversation

@ChaturvediShilpa
Copy link
Contributor

@ChaturvediShilpa ChaturvediShilpa commented Jul 17, 2025

  • Add JSONLeveledLogger and JSONLoggerFactory for structured JSON logging
  • Support all log levels (TRACE, DEBUG, INFO, WARN, ERROR, DISABLED)
  • Maintain backward compatibility with existing text logging
  • Add comprehensive tests for JSON logging functionality
  • Update documentation with usage examples
  • Require Go 1.21+ for slog library support

This change allows users to choose between text and JSON logging formats while maintaining the same API and functionality.

Description

Reference issue

Closes #127

@Sean-Der
Copy link
Member

Would it better to deprecate pion/logging entirely? Now that slog exists this is just cruft/indirection I believe

@JoTurk
Copy link
Member

JoTurk commented Jul 17, 2025

Would it better to deprecate pion/logging entirely? Now that slog exists this is just cruft/indirection I believe

Maybe when we upgrade to 1.21?

@rg0now
Copy link

rg0now commented Jul 28, 2025

I'd love to get this or a similar functionality merged (I haven't reviewed the code yet). Structured logging is one of the most requested features in STUNner given how pretty much everything in the Kubernetes ecosystem is built around this logging style.

Maybe we could save this PR by using/writing some slog replacement/wrapper that works with Golang v1.20 for the time pion is stuck at that Go version, and then transitioning to the official slog package once we make the switch. But we could also choose whatever structured logging lib we see fit. Of course, this will still be a half-solution only, given how pion/logging is built-around Printf style logging to format parameters, which is in fundamental violation of the philosophy of structured logging.

That being said, deprecating pion/logging would put a pretty high burden on upstream projects that depend on it. I'm not saying we shouldn't do that, but I believe we should definitely have some smoother upgrade path in place before we make that move, like the compatibility layer suggested in this PR.

@JoTurk
Copy link
Member

JoTurk commented Jul 28, 2025

Maybe we could save this PR by using/writing some slog replacement/wrapper that works with Golang v1.20 for the time pion is stuck at that Go version, and then transitioning to the official slog package once we make the switch. But we could also choose whatever structured logging lib we see fit. Of course, this will still be a half-solution only, given how pion/logging is built-around Printf style logging to format parameters, which is in fundamental violation of the philosophy of structured logging.

I like this idea, I can contribute to this, if we go with this idea.

@JoTurk
Copy link
Member

JoTurk commented Oct 10, 2025

@ChaturvediShilpa Hello we upgraded to go 1.21, do you wanna get this merged?

@JoTurk JoTurk force-pushed the logging-json-format branch from 182e355 to 438adda Compare October 23, 2025 05:33
Copy link
Member

@JoTurk JoTurk left a comment

Choose a reason for hiding this comment

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

Do you want to fix the errors in the compile example so we can merge this? otherwise i can fix them myself.
Thank you.

json_logger.go Outdated
// Create structured log entry
attrs := []any{
"scope", jl.scope,
"level", jl.pionLevelToString(jl.logLevelToPionLevel(level)),
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this lead to duplicated levels?

@philipch07 philipch07 force-pushed the logging-json-format branch from 438adda to 13a6fdf Compare January 2, 2026 23:24
@codecov
Copy link

codecov bot commented Jan 2, 2026

Codecov Report

❌ Patch coverage is 80.64516% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.36%. Comparing base (32a8ea5) to head (a70ea01).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
json_logger.go 80.64% 23 Missing and 7 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
- Coverage   98.33%   88.36%   -9.97%     
==========================================
  Files           2        3       +1     
  Lines         120      275     +155     
==========================================
+ Hits          118      243     +125     
- Misses          1       24      +23     
- Partials        1        8       +7     
Flag Coverage Δ
go 88.36% <80.64%> (-9.97%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@philipch07 philipch07 force-pushed the logging-json-format branch 2 times, most recently from e3b8ae9 to 76ffa05 Compare January 3, 2026 00:03
- Add structured JSON logging for all log levels
- Update documentation with usage examples
- Require Go 1.21+ for slog library support

Users can now choose between text and JSON logs
while maintaining the same API and functionality.
@philipch07 philipch07 force-pushed the logging-json-format branch 4 times, most recently from fd09553 to 99df69b Compare January 4, 2026 23:01
@philipch07 philipch07 force-pushed the logging-json-format branch 2 times, most recently from 1d4cbe0 to d5223fe Compare January 6, 2026 03:46
@philipch07 philipch07 force-pushed the logging-json-format branch from d5223fe to af0ba73 Compare January 7, 2026 21:59
@rg0now
Copy link

rg0now commented Jan 9, 2026

Overall, I really like this. I think the JSONLoggerFactory will be a really nice drop-in replacement for people who insist on structured logging without having to change anything in the rest of the code depending on pion/logging. Just to make sure that *JSONLoggerFactory is a proper LoggerFactory, can you add an assertion like the below to json_logger.go?

var _ LoggerFactory = NewJSONLoggerFactory()

Apart, I think (to the extent I could review the code) this can be merged.

@JoTurk
Copy link
Member

JoTurk commented Jan 9, 2026

@rg0now yeah we talked (me and @philipch07 ) about making sure that JSONLoggerFactory is LoggerFactory before merging this. @philipch07 asked me to do it and i was doing it tonight.

@JoTurk
Copy link
Member

JoTurk commented Jan 9, 2026

Also making all the unnecessary interfaces / structs private.

@JoTurk
Copy link
Member

JoTurk commented Jan 9, 2026

@philipch07 @rg0now I pushed a commit to private everything that we don't need, added a LoggerFactory interface, made sure we make LeveledLogger. can you please give it a review when you have time?

@philipch07
Copy link
Contributor

Thanks for cleaning it up! It looks good to me :)

@JoTurk
Copy link
Member

JoTurk commented Jan 9, 2026

@rg0now does this look good to you?

@JoTurk JoTurk merged commit 6dc79c9 into pion:master Jan 11, 2026
15 of 16 checks passed
@JoTurk
Copy link
Member

JoTurk commented Jan 11, 2026

@rg0now I'm going to test the logging factory again before tagging this, but i think it should be drop in replacement now.

thank you @ChaturvediShilpa @philipch07

@rg0now
Copy link

rg0now commented Jan 12, 2026

Sorry, missed this thread. This was a long-standing issue and I'm really happy it finally got fixed. Thanks!

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Enable json based logging support for Pion

5 participants