-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add support for sending flag decisions along with decision metadata. #292
Conversation
3f78725
to
81b6651
Compare
pkg/event/factory.go
Outdated
|
||
var variationID string | ||
if variation != nil { | ||
metadata["VARIAITON_KEY"] = variation.Key |
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.
nit: should be VARIATION_KEY
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.
We've exposed the flaw behind open-ended maps :P
pkg/event/factory.go
Outdated
} | ||
|
||
// CreateImpressionUserEvent creates and returns ImpressionEvent for user | ||
func CreateImpressionUserEvent(projectConfig config.ProjectConfig, experiment entities.Experiment, | ||
variation entities.Variation, | ||
userContext entities.UserContext) UserEvent { | ||
variation *entities.Variation, userContext entities.UserContext, flagKey, flagType string) (UserEvent, bool) { |
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.
ٍvariation *entities.Variation
won't it break existing apps? e.g. Agent
?
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.
Unfortunately, this is public only due to our package structure and is technically should be a private function.
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.
FWIW, Agent does not call this directly
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.
got it.
pkg/event/events.go
Outdated
EntityID string `json:"entity_id"` | ||
ExperimentID string `json:"experiment_id"` | ||
Key string `json:"key"` | ||
Metadata map[string]string `json:"metadata"` |
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.
strictly
typed representation would be great here.
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've replaced this with a struct
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.
lgtm
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.
lgtm
type DecisionMetadata struct { | ||
FlagType string `json:"flag_type"` | ||
FlagKey string `json:"flag_key"` | ||
VariationKey string `json:"variation_key"` |
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.
shouldn't this be defined with omitempty
since this is an optional value?
VariationKey string `json:"variation_key"` | |
VariationKey string `json:"variation_key,omitempty"` |
Summary
Metadata
field to EventBatch.Decisions to capture flag type, key and variation key.This change outline the proposed logical and structural changes required to support capturing all decision instances. The new eventbatch field is backwards compatible with the current pipeline.