-
Notifications
You must be signed in to change notification settings - Fork 39
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
Feature: event system and impression data events #113
Changes from all commits
3bd8fc7
649d95c
29bf1f2
30fa6fc
a3ac7d5
9992f04
4154a01
4addefd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,6 +78,8 @@ public DefaultUnleash(UnleashSettings settings, bool overrideDefaultStrategies, | |
/// <inheritdoc /> | ||
public ICollection<FeatureToggle> FeatureToggles => services.ToggleCollection.Instance.Features; | ||
|
||
private EventCallbackConfig EventConfig { get; set; } | ||
|
||
/// <inheritdoc /> | ||
public bool IsEnabled(string toggleName) | ||
{ | ||
|
@@ -125,6 +127,9 @@ private bool CheckIsEnabled(string toggleName, UnleashContext context, bool defa | |
} | ||
|
||
RegisterCount(toggleName, enabled); | ||
|
||
if (featureToggle?.ImpressionData ?? false) EmitImpressionEvent("isEnabled", context, enabled, featureToggle.Name); | ||
|
||
return enabled; | ||
} | ||
|
||
|
@@ -146,6 +151,9 @@ public Variant GetVariant(string toggleName, UnleashContext context, Variant def | |
var variant = enabled ? VariantUtils.SelectVariant(toggle, context, defaultValue) : defaultValue; | ||
|
||
RegisterVariant(toggleName, variant); | ||
|
||
if (toggle?.ImpressionData ?? false) EmitImpressionEvent("getVariant", context, enabled, toggle.Name, variant.Name); | ||
|
||
return variant; | ||
} | ||
|
||
|
@@ -235,6 +243,52 @@ private IEnumerable<Constraint> ResolveConstraints(ActivationStrategy activation | |
} | ||
} | ||
|
||
public void ConfigureEvents(Action<EventCallbackConfig> callback) | ||
{ | ||
if (callback == null) | ||
{ | ||
Logger.Error($"UNLEASH: Unleash->ConfigureEvents parameter callback is null"); | ||
return; | ||
} | ||
|
||
try | ||
{ | ||
var evtConfig = new EventCallbackConfig(); | ||
callback(evtConfig); | ||
EventConfig = evtConfig; | ||
} | ||
catch (Exception ex) | ||
{ | ||
Logger.Error($"UNLEASH: Unleash->ConfigureEvents executing callback threw exception: {ex.Message}"); | ||
} | ||
} | ||
|
||
private void EmitImpressionEvent(string type, UnleashContext context, bool enabled, string name, string variant = null) | ||
{ | ||
if (EventConfig.ImpressionEvent == null) | ||
{ | ||
Logger.Error($"UNLEASH: Unleash->ImpressionData callback is null, unable to emit event"); | ||
return; | ||
} | ||
|
||
try | ||
{ | ||
EventConfig.ImpressionEvent(new ImpressionEvent | ||
{ | ||
Type = type, | ||
Context = context, | ||
EventId = Guid.NewGuid().ToString(), | ||
Enabled = enabled, | ||
FeatureName = name, | ||
Variant = variant | ||
}); | ||
} | ||
catch (Exception ex) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above, I can't see an obvious exception that could be caught here, is this just being defensive? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here I'm trapping event emitting, if your logger fails for some reason for instance, that won't crash harder than an error in the log There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I can live with that, seems polite to log an error if something super unexpected happens here |
||
{ | ||
Logger.Error($"UNLEASH: Emitting impression event callback threw exception: {ex.Message}"); | ||
} | ||
} | ||
|
||
public void Dispose() | ||
{ | ||
services?.Dispose(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Text; | ||
|
||
namespace Unleash.Internal | ||
{ | ||
public class EventCallbackConfig | ||
{ | ||
public Action<ImpressionEvent> ImpressionEvent { get; set; } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Text; | ||
|
||
namespace Unleash.Internal | ||
{ | ||
public class ImpressionEvent | ||
{ | ||
public string Type { get; set; } | ||
public string EventId { get; set; } | ||
public UnleashContext Context { get; set; } | ||
public bool Enabled { get; set; } | ||
public string FeatureName { get; set; } | ||
public string Variant { get; set; } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
{ | ||
"version": 2, | ||
"features": [ | ||
{ | ||
"name": "Tests only", | ||
"description": "Where name has test in it", | ||
"enabled": true, | ||
"impressionData": true, | ||
"strategies": [ | ||
{ | ||
"name": "default", | ||
"parameters": {}, | ||
"segments": [ 1 ] | ||
} | ||
] | ||
} | ||
], | ||
"segments": [ | ||
{ | ||
"id": 1, | ||
"constraints": [ | ||
{ | ||
"contextName": "name", | ||
"operator": "STR_CONTAINS", | ||
"value": "test" | ||
} | ||
] | ||
} | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
using FluentAssertions; | ||
using NUnit.Framework; | ||
using System; | ||
|
||
namespace Unleash.Tests | ||
{ | ||
public class DefaultUnleashTests | ||
{ | ||
[Test] | ||
public void ConfigureEvents_should_invoke_callback() | ||
{ | ||
// Arrange | ||
var settings = new UnleashSettings | ||
{ | ||
AppName = "testapp", | ||
}; | ||
|
||
var unleash = new DefaultUnleash(settings); | ||
var callbackCalled = false; | ||
|
||
// Act | ||
unleash.ConfigureEvents(cfg => | ||
{ | ||
callbackCalled = true; | ||
}); | ||
|
||
// Assert | ||
callbackCalled.Should().BeTrue(); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there something that can be raised here? This looks pretty harmless to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular one is probably very harmless, it's the callback where you configure all the handlers for the various events. But anything started in here that fails on a file lock or whatever will crash the startup of Unleash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can stand by that!