Skip to content

log: make sink encodable#5441

Merged
mholt merged 6 commits intomasterfrom
jsonify-sink-log
Mar 27, 2023
Merged

log: make sink encodable#5441
mholt merged 6 commits intomasterfrom
jsonify-sink-log

Conversation

@mohammed90
Copy link
Member

sink logs have, thus far, been printed not encoded in any format. This does not play friendly with log parsers expecting a specific format from a particular source. In other words, it becomes a hassle to tell the log parser, e.g. Loki, Caddy logs are json-formated if some lines aren't.

@mohammed90 mohammed90 added feature ⚙️ New feature or request under review 🧐 Review is pending before merging needs docs ✍️ Requires documentation changes labels Mar 17, 2023
@mohammed90 mohammed90 added this to the v2.7.0 milestone Mar 17, 2023
@mohammed90 mohammed90 requested a review from mholt March 17, 2023 14:13
@mholt
Copy link
Member

mholt commented Mar 17, 2023

I wonder if we can embed one type within another (StandardLibLog or CustomLog) to reduce duplication. If StandardLibLog used CustomLog, I'd be pretty confident that the change is good 👍

@mohammed90
Copy link
Member Author

mohammed90 commented Mar 19, 2023

I wonder if we can embed one type within another (StandardLibLog or CustomLog) to reduce duplication. If StandardLibLog used CustomLog, I'd be pretty confident that the change is good 👍

Now that I've tried it, embedding introduces more invasive changes than the duplication. I even tried to have a third struct type for the common fields, but same limitation incurred, keyed fields had to be wrapped with the embedded type. The CustomLogger cannot be the one that's embedded because it contains Include and Exclude fields which don't operate outside of it and have no semantics for anything besides it.

I don't like how invasive this attempt at duplication. I wonder if there's another way by means of an unexported function.

In case you want to play with it, here's a working config:

{
	"logging": {
		"sink" : {
			"level": "DEBUG",
			"encoder": {
				"format": "json"
			}
		},
		"logs": {
			"default": {
				"level": "DEBUG",
				"encoder": {
					"format": "json"
				}
			}
		}
	},
	"apps": {
		"http": {
			"servers": {
				"srv0": {
					"listen": [
						":443"
					],
					"routes": [
						{
							"match": [
								{
									"host": [
										"localhost"
									]
								}
							],
							"handle": [
								{
									"handler": "subroute",
									"routes": [
										{
											"handle": [
												{
													"body": "Howdy!",
													"handler": "static_response"
												}
											]
										}
									]
								}
							],
							"terminal": true
						}
					]
				}
			}
		}
	}
}

@francislavoie
Copy link
Member

I highly doubt any plugin is directly using the StandardLibLog type right now. Could we just rename it to BaseLog or something, and use that as sink? That would make it read less weird.

@mohammed90
Copy link
Member Author

I highly doubt any plugin is directly using the StandardLibLog type right now. Could we just rename it to BaseLog or something, and use that as sink? That would make it read less weird.

How about now?

Copy link
Member

@francislavoie francislavoie left a comment

Choose a reason for hiding this comment

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

Looks great to me 👍

I want Matt's review before merging tho

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Awesome -- yeah, let's give it a try!

@mholt mholt enabled auto-merge (squash) March 27, 2023 21:04
@mholt mholt removed the under review 🧐 Review is pending before merging label Mar 27, 2023
@mholt mholt merged commit 1aef807 into master Mar 27, 2023
@mholt mholt deleted the jsonify-sink-log branch March 27, 2023 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature ⚙️ New feature or request needs docs ✍️ Requires documentation changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants