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

Make user-supplied sinks operate on URIs #606

Merged
merged 6 commits into from
Jul 19, 2018
Merged

Make user-supplied sinks operate on URIs #606

merged 6 commits into from
Jul 19, 2018

Conversation

akshayjshah
Copy link
Contributor

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
Copy link

codecov bot commented Jun 29, 2018

Codecov Report

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

Impacted file tree graph

@@            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.

Copy link
Collaborator

@prashantv prashantv 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 the slow review, change looks good. Just some small suggestions

config.go Outdated
@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we mention that filenames are also supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, can do.

sink.go Outdated
_sinkMutex.RLock()
defer _sinkMutex.RUnlock()
sinkFactory, ok := _sinkFactories[key]

factory, ok := _sinkFactories[u.Scheme]
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, can do.

}

type nopCloserSink struct{ zapcore.WriteSyncer }
func newFileSink(u *url.URL) (Sink, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@akshayjshah
Copy link
Contributor Author

akshayjshah commented Jul 13, 2018

@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.

sink.go Outdated
// 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: "must not"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, fixed.

sink.go Outdated
// (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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@abhinav
Copy link
Collaborator

abhinav commented Jul 13, 2018

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
Copy link
Contributor Author

akshayjshah commented Jul 19, 2018

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.

@akshayjshah akshayjshah merged commit a01e410 into master Jul 19, 2018
@abhinav abhinav deleted the ajs/defer branch September 2, 2020 16:54
cgxxv pushed a commit to cgxxv/zap that referenced this pull request Mar 25, 2022
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.
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.

4 participants