Make user-supplied sinks operate on URIs #606

akshayjshah · 2018-06-29T23:28:26Z

For future extensibility, make user-supplied factories for log sinks
operate on URIs. Each user-supplied factory owns a scheme, and
double-registering constructors for a scheme is an error. For
back-compat, zap automatically registers a factory for the file scheme
and treats URIs without a scheme as though they were for files.

Small formatting change for readability.

For future extensibility, make user-supplied factories for log sinks operate on URIs. Each user-supplied factory owns a scheme, and double-registering constructors for a scheme is an error. For back-compat, zap automatically registers a factory for the `file` scheme and treats URIs without a scheme as though they were for files.

codecov · 2018-06-29T23:34:47Z

Codecov Report

Merging #606 into master will increase coverage by 0.13%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #606      +/-   ##
==========================================
+ Coverage   97.23%   97.37%   +0.13%     
==========================================
  Files          40       40              
  Lines        2063     2095      +32     
==========================================
+ Hits         2006     2040      +34     
+ Misses         49       47       -2     
  Partials        8        8

Impacted Files	Coverage Δ
config.go	`97.91% <ø> (ø)`	⬆️
writer.go	`100% <100%> (ø)`	⬆️
sink.go	`100% <100%> (+7.4%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e7e266...c7f4f5e. Read the comment docs.

prashantv

Sorry for the slow review, change looks good. Just some small suggestions

prashantv · 2018-07-05T16:38:40Z

config.go

@@ -74,10 +74,10 @@ type Config struct {
 	// EncoderConfig sets options for the chosen encoder. See
 	// zapcore.EncoderConfig for details.
 	EncoderConfig zapcore.EncoderConfig `json:"encoderConfig" yaml:"encoderConfig"`
-	// OutputPaths is a list of paths to write logging output to. See Open for
+	// OutputPaths is a list of URLs to write logging output to. See Open for


Should we mention that filenames are also supported?

Sure, can do.

prashantv · 2018-07-05T16:44:01Z

sink.go

 	_sinkMutex.RLock()
 	defer _sinkMutex.RUnlock()
-	sinkFactory, ok := _sinkFactories[key]
+
+	factory, ok := _sinkFactories[u.Scheme]


optionally: can we hold the RLock just while we're looking up the factory?

_sinkMutex.RLock() factory, ok := _sinkFactories[u.Scheme] _sinkMutex.RUnlock()

Shouldn't be an issue since it's just an RLock, but want to avoid holding on to locks while triggering the factory which is user-supplied.

Makes sense, can do.

prashantv · 2018-07-05T16:47:17Z

sink.go

 }

-type nopCloserSink struct{ zapcore.WriteSyncer }
+func newFileSink(u *url.URL) (Sink, error) {


optional: should we ensure there's no fragments etc? ignoring seems OK too, but it might be a little surprising

Good point.

prashantv · 2018-07-05T16:50:28Z

sink.go

+		case c == '.' || c == '+' || c == '-':
+			continue
+		}
+		return "", fmt.Errorf("may not contain %q", string(c))


%q works fine for characters, it does single quotes instead of double quotes, which seems fine here?

Definitely.

prashantv · 2018-07-05T16:52:52Z

sink.go

+	if first := s[0]; 'a' > first || 'z' < first {
+		return "", errors.New("must start with a letter")
+	}
+	if len(s) < 2 {


the rest of the code will work fine for len(s) == 1, any reason for this special case?

Nope, just left over from previous iterations on the code. My bad.

akshayjshah · 2018-07-13T06:17:12Z

@prashantv Cleaned up per your suggestions. Validating file URLs more strictly was a really good idea. I'll leave this open for a few days in case you'd like to re-review.

abhinav · 2018-07-13T17:29:57Z

sink.go

+// particular scheme.
+//
+// All schemes must be ASCII, valid under section 3.1 of RFC 3986
+// (https://tools.ietf.org/html/rfc3986#section-3.1), and may not already have


nit: "must not"

Good point, fixed.

abhinav · 2018-07-13T17:35:59Z

sink.go

+// (https://tools.ietf.org/html/rfc3986#section-3.1), and may not already have
+// a factory registered. Zap automatically registers a factory for the "file"
+// scheme.
+func RegisterSink(scheme string, factory func(*url.URL) (Sink, error)) error {


Isn't this a breaking change? The factory function didn't accept a callback before.

abhinav · 2018-07-13T17:50:02Z

I've been informed that this hasn't been released yet so it isn't a breaking
change. LGTM then.

Do you think we'll ever need to change this signature in the future? If yes,
consider making the sink factory an interface so that we can declare a new
interface and upcast to it if we ever need to change the arguments we're
passing to factory. Although we can also declare a new RegisterSink*
function in that case, so this is up to you.

akshayjshah · 2018-07-19T21:04:31Z

There's always a chance that we'll need to change this later; in that case, though, I don't see much difference between introducing a new interface and introducing a new registration function - in the latter case, we can keep the interface with both methods unexported.

The current approach matches RegisterEncoder, which is a nice bit of symmetry.

For future extensibility, make user-supplied factories for log sinks operate on URIs. Each user-supplied factory owns a scheme, and double-registering constructors for a scheme is an error. For back-compat, zap automatically registers a factory for the `file` scheme and treats URIs without a scheme as though they were for files.

akshayjshah added 2 commits June 29, 2018 13:21

Newlines after defers

f8b6d35

Small formatting change for readability.

akshayjshah requested review from abhinav and prashantv June 29, 2018 23:28

Fix import ordering

0db234b

akshayjshah mentioned this pull request Jun 30, 2018

Ability to build logger with custom Sink #572

Merged

dimroc approved these changes Jun 30, 2018

View reviewed changes

prashantv approved these changes Jul 9, 2018

View reviewed changes

akshayjshah added 2 commits July 12, 2018 23:13

Disallow filepaths with fragments, queries, etc

6e33ddf

Clean up normalizeScheme

8dccd5c

abhinav reviewed Jul 13, 2018

View reviewed changes

Fix RFC terminology

c7f4f5e

akshayjshah merged commit a01e410 into master Jul 19, 2018

Apeopex mentioned this pull request Aug 13, 2018

Windows paths not supported by default file sink #621

Open

abhinav deleted the ajs/defer branch September 2, 2020 16:54

github-actions bot mentioned this pull request Apr 10, 2022

Link Checker Report scylladb/scylla-manager#3102

Closed

github-actions bot mentioned this pull request Apr 17, 2022

Link Checker Report scylladb/scylla-manager#3121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make user-supplied sinks operate on URIs #606

Make user-supplied sinks operate on URIs #606

akshayjshah commented Jun 29, 2018

codecov bot commented Jun 29, 2018 •

edited

Loading

prashantv left a comment

prashantv Jul 5, 2018

akshayjshah Jul 12, 2018

prashantv Jul 5, 2018

akshayjshah Jul 12, 2018

prashantv Jul 5, 2018

akshayjshah Jul 12, 2018

prashantv Jul 5, 2018

akshayjshah Jul 13, 2018

prashantv Jul 5, 2018

akshayjshah Jul 13, 2018

akshayjshah commented Jul 13, 2018 •

edited

Loading

abhinav Jul 13, 2018

akshayjshah Jul 19, 2018

abhinav Jul 13, 2018

abhinav commented Jul 13, 2018 •

edited

Loading

akshayjshah commented Jul 19, 2018 •

edited

Loading

Make user-supplied sinks operate on URIs #606

Make user-supplied sinks operate on URIs #606

Conversation

akshayjshah commented Jun 29, 2018

codecov bot commented Jun 29, 2018 • edited Loading

Codecov Report

prashantv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshayjshah commented Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinav commented Jul 13, 2018 • edited Loading

akshayjshah commented Jul 19, 2018 • edited Loading

codecov bot commented Jun 29, 2018 •

edited

Loading

akshayjshah commented Jul 13, 2018 •

edited

Loading

abhinav commented Jul 13, 2018 •

edited

Loading

akshayjshah commented Jul 19, 2018 •

edited

Loading