HDE-14 audit logging support #8
Conversation
VERSION
Outdated
| @@ -1 +1 @@ | |||
| 1.2.1 | |||
| 1.2.2 | |||
There was a problem hiding this comment.
Doesn't match version in Changelog. Seems like 1.3.0 is correct as this is adding functionality.
There was a problem hiding this comment.
Oops sorry about that
There was a problem hiding this comment.
No worries, easy to fix!
| @@ -28,6 +29,8 @@ type RecordEventCallArgs struct { | |||
| } | |||
|
|
|||
| type StructuredOutputLogProvider struct { | |||
| providers.LogProvider | |||
There was a problem hiding this comment.
This is actually the first time I've looked at the way we do log chaining, so let me know if this is incorrect, but does the next provider have to be explicitly called in all the various log methods below? (Not seeing it being invoked elsewhere.) I.e., in func (p *StructuredOutputLogProvider) Error(...), you'd call
if p.nextProvider != nil {
p.nextProvider.Error(ctx, report, args...)
}
And same for the other six methods defined in LogProvider.
There was a problem hiding this comment.
I think you're right. Otherwise, it wouldn't really be chaining I guess.
There was a problem hiding this comment.
Sounds good- feel free to check that with Chris if there's any ambiguity, since there could be something I'm missing.
providers/structured/provider.go
Outdated
| rawLogCalls: rawLogCalls, | ||
| recordCalls: []RecordCallArgs{}, | ||
| recordEventCalls: []RecordEventCallArgs{}, | ||
| } | ||
|
|
||
| if nextProvider != nil { |
There was a problem hiding this comment.
Possible to test the behavior of this block in the unit tests? The rest of this method is already covered, so you'd just need to add a test for nextProvider == nil/another for nextProvider != nil.
| "Metric 3": "Value 3", | ||
| "Metric 4": "Value 4", | ||
| }) | ||
|
|
There was a problem hiding this comment.
Think you should also test that calling lp.Wait() causes lp.nextProvider.Wait() to be called. Looks like you could use dummy/provider as essentially a mock for nextProvider, to accomplish that verification.
|
Thanks for all the help @jpecknerhelix |
| @@ -186,6 +186,17 @@ var _ = Describe("bufferedLogProvider", func() { | |||
| Ω(lp.LogProvider).Should(Equal(chaining.LogProvider(dummyProvider))) | |||
| }) | |||
|
|
|||
| It("Should call wait on the next provider", func() { | |||
| ws := new(dummy.WaitState) | |||
There was a problem hiding this comment.
Nit: add Ω(ws.Get()).Should(BeFalse()) after this line as a sanity check
|
No problem! Btw, are the unit tests for the |
|
^ @emreathelix, CC: @chriswhelix |
|
@jpecknerhelix All tests are passing for me both on master and this branch. Make sure you've run |
|
Weird, I'm actually seeing more errors after running |
|
Created a ticket in JIRA for the failing test issue. |
log/log.go
Outdated
| @@ -24,6 +24,10 @@ func SetDefaultProvider(provider providers.LogProvider) { | |||
| defaultProvider = provider | |||
| } | |||
|
|
|||
| func GetDefaultProvider() providers.LogProvider { | |||
There was a problem hiding this comment.
Idiomatic name in Go should be just DefaultProvider().
providers/structured/provider.go
Outdated
| rawLogCalls: rawLogCalls, | ||
| recordCalls: []RecordCallArgs{}, | ||
| recordEventCalls: []RecordEventCallArgs{}, | ||
| } | ||
|
|
||
| if nextProvider != nil { | ||
| lp.LogProvider = chaining.LogProvider(nextProvider) |
There was a problem hiding this comment.
What you want to do is here always set lp.LogProvider to the chaining provider, even if nextProvider is nil; then you don't need to bother with all the nil-checking in individual methods, because the chaining provider does it for you (that's its main job).
There was a problem hiding this comment.
Ah ok.. Didn't check the internals of the chaining provider.
providers/structured/provider.go
Outdated
| if p.LogProvider != nil { | ||
| p.LogProvider.Wait() | ||
| } | ||
| } |
There was a problem hiding this comment.
You don't need to implement this at all; the chaining provider will give you an empty default implementation.
providers/structured/provider.go
Outdated
| return LogProvider(nil).(*StructuredOutputLogProvider) | ||
| } | ||
|
|
||
| func LogProvider(nextProvider providers.LogProvider) providers.LogProvider { |
There was a problem hiding this comment.
Why do we need two different constructors here, with different signatures?
There was a problem hiding this comment.
Because NewStructuredOutputLogProvider() is already being used in other services. I didn't wanna break any existing functionality.
There was a problem hiding this comment.
Hmm, OK. Let's mark the old one deprecated using the Go convention (see: https://rakyll.org/deprecated/), and have this also return *StructuredOutputLogProvider to avoid awkward casting of its result.
No description provided.