-
Notifications
You must be signed in to change notification settings - Fork 63
Publish raw events #151
Publish raw events #151
Changes from 4 commits
9306023
6fa0eab
3e44470
33ed060
7c2a395
3b170a1
9224c6c
1f91d61
e242acb
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 |
---|---|---|
@@ -0,0 +1,98 @@ | ||
package implementations | ||
|
||
import ( | ||
"context" | ||
"strings" | ||
|
||
"github.com/lyft/flyteidl/gen/pb-go/flyteidl/admin" | ||
|
||
"github.com/lyft/flyteadmin/pkg/async/notifications/interfaces" | ||
|
||
"github.com/NYTimes/gizmo/pubsub" | ||
"github.com/golang/protobuf/proto" | ||
"github.com/lyft/flytestdlib/logger" | ||
"github.com/lyft/flytestdlib/promutils" | ||
"github.com/prometheus/client_golang/prometheus" | ||
) | ||
|
||
type eventPublisherSystemMetrics struct { | ||
Scope promutils.Scope | ||
PublishTotal prometheus.Counter | ||
PublishError prometheus.Counter | ||
} | ||
|
||
// TODO: Add a counter that encompasses the publisher stats grouped by project and domain. | ||
type EventPublisher struct { | ||
pub pubsub.Publisher | ||
systemMetrics eventPublisherSystemMetrics | ||
events []string | ||
} | ||
|
||
func getSupportedEvents() map[string]string { | ||
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. nit: instead of using a func you can actually initialize the map in golang as a var. see https://stackoverflow.com/questions/41078272/initializing-a-go-map-in-a-single-statement for an example |
||
supportedEvents := make(map[string]string) | ||
var taskExecutionReq admin.TaskExecutionEventRequest | ||
supportedEvents["task"] = proto.MessageName(&taskExecutionReq) | ||
var nodeExecutionReq admin.NodeExecutionEventRequest | ||
supportedEvents["node"] = proto.MessageName(&nodeExecutionReq) | ||
var workflowExecutionReq admin.WorkflowExecutionEventRequest | ||
supportedEvents["workflow"] = proto.MessageName(&workflowExecutionReq) | ||
return supportedEvents | ||
} | ||
|
||
// The key is the notification type as defined as an enum. | ||
func (p *EventPublisher) Publish(ctx context.Context, notificationType string, msg proto.Message) error { | ||
p.systemMetrics.PublishTotal.Inc() | ||
logger.Debugf(ctx, "Publishing the following message [%s]", msg.String()) | ||
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. does String() return a human readable message? if not can we use |
||
|
||
if !p.shouldPublishEvent(notificationType) { | ||
return nil | ||
} | ||
|
||
err := p.pub.Publish(ctx, notificationType, msg) | ||
if err != nil { | ||
p.systemMetrics.PublishError.Inc() | ||
logger.Errorf(ctx, "Failed to publish a message with key [%s] and message [%s] and error: %v", notificationType, msg.String(), err) | ||
} | ||
return err | ||
} | ||
|
||
func (p *EventPublisher) shouldPublishEvent(notificationType string) bool { | ||
for _, e := range p.events { | ||
if e == notificationType { | ||
return true | ||
} | ||
} | ||
return false | ||
} | ||
|
||
func newEventPublisherSystemMetrics(scope promutils.Scope) eventPublisherSystemMetrics { | ||
return eventPublisherSystemMetrics{ | ||
Scope: scope, | ||
PublishTotal: scope.MustNewCounter("event_publish_total", "overall count of publish messages"), | ||
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. i know it's all additive, but can we add a success counter too? |
||
PublishError: scope.MustNewCounter("event_publish_errors", "count of publish errors"), | ||
} | ||
} | ||
|
||
func NewEventsPublisher(pub pubsub.Publisher, scope promutils.Scope, eventTypes string) interfaces.Publisher { | ||
supportedEvents := getSupportedEvents() | ||
|
||
var eventList = make([]string, 0) | ||
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. do you want to use a String set here instead since you only ever check for membership/ |
||
if strings.Contains(eventTypes, "*") || strings.Contains(eventTypes, "all") { | ||
for _, e := range supportedEvents { | ||
eventList = append(eventList, e) | ||
} | ||
} else { | ||
events := strings.Split(eventTypes, ",") | ||
for _, event := range events { | ||
if e, found := supportedEvents[event]; found { | ||
eventList = append(eventList, e) | ||
} | ||
} | ||
katrogan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
return &EventPublisher{ | ||
pub: pub, | ||
systemMetrics: newEventPublisherSystemMetrics(scope.NewSubScope("events_publisher")), | ||
events: eventList, | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,173 @@ | ||
package implementations | ||
|
||
import ( | ||
"context" | ||
"errors" | ||
"testing" | ||
"time" | ||
|
||
"github.com/golang/protobuf/ptypes" | ||
"github.com/lyft/flyteidl/gen/pb-go/flyteidl/admin" | ||
"github.com/lyft/flyteidl/gen/pb-go/flyteidl/core" | ||
"github.com/lyft/flyteidl/gen/pb-go/flyteidl/event" | ||
|
||
"github.com/NYTimes/gizmo/pubsub" | ||
"github.com/NYTimes/gizmo/pubsub/pubsubtest" | ||
"github.com/golang/protobuf/proto" | ||
"github.com/lyft/flytestdlib/promutils" | ||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
var testEventPublisher pubsubtest.TestPublisher | ||
var mockEventPublisher pubsub.Publisher = &testEventPublisher | ||
|
||
var executionID = core.WorkflowExecutionIdentifier{ | ||
Project: "project", | ||
Domain: "domain", | ||
Name: "name", | ||
} | ||
var nodeExecutionID = core.NodeExecutionIdentifier{ | ||
NodeId: "node id", | ||
ExecutionId: &executionID, | ||
} | ||
|
||
var taskID = &core.Identifier{ | ||
ResourceType: core.ResourceType_TASK, | ||
Project: "p", | ||
Domain: "d", | ||
Version: "v", | ||
Name: "n", | ||
} | ||
|
||
var occurredAt = time.Now().UTC() | ||
var occurredAtProto, _ = ptypes.TimestampProto(occurredAt) | ||
|
||
var taskPhase = core.TaskExecution_RUNNING | ||
|
||
var retryAttempt = uint32(1) | ||
|
||
const requestID = "request id" | ||
|
||
var taskRequest = &admin.TaskExecutionEventRequest{ | ||
RequestId: requestID, | ||
Event: &event.TaskExecutionEvent{ | ||
TaskId: taskID, | ||
ParentNodeExecutionId: &nodeExecutionID, | ||
RetryAttempt: retryAttempt, | ||
Phase: taskPhase, | ||
OccurredAt: occurredAtProto, | ||
}, | ||
} | ||
|
||
var nodeRequest = &admin.NodeExecutionEventRequest{ | ||
RequestId: requestID, | ||
Event: &event.NodeExecutionEvent{ | ||
ProducerId: "propeller", | ||
Id: &nodeExecutionID, | ||
OccurredAt: occurredAtProto, | ||
Phase: core.NodeExecution_RUNNING, | ||
InputUri: "input uri", | ||
}, | ||
} | ||
|
||
var workflowRequest = &admin.WorkflowExecutionEventRequest{ | ||
Event: &event.WorkflowExecutionEvent{ | ||
Phase: core.WorkflowExecution_SUCCEEDED, | ||
OutputResult: &event.WorkflowExecutionEvent_OutputUri{ | ||
OutputUri: "somestring", | ||
}, | ||
ExecutionId: &executionID, | ||
}, | ||
} | ||
|
||
// This method should be invoked before every test around Publisher. | ||
func initializeEventPublisher() { | ||
testEventPublisher.Published = nil | ||
testEventPublisher.GivenError = nil | ||
testEventPublisher.FoundError = nil | ||
} | ||
|
||
func TestNewEventsPublisher_EventTypes(t *testing.T) { | ||
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. Thank you for this very exhaustive test 👍 |
||
{ | ||
tests := []struct { | ||
name string | ||
eventTypes string | ||
events []proto.Message | ||
shouldSendEvent []bool | ||
expectedSendCnt int | ||
}{ | ||
{"eventTypes as workflow,node", "workflow,node", | ||
[]proto.Message{workflowRequest, nodeRequest, taskRequest}, | ||
[]bool{true, true, false}, | ||
2}, | ||
{"eventTypes as workflow,task", "workflow,task", | ||
[]proto.Message{workflowRequest, nodeRequest, taskRequest}, | ||
[]bool{true, false, true}, | ||
2}, | ||
{"eventTypes as workflow,task", "node,task", | ||
[]proto.Message{workflowRequest, nodeRequest, taskRequest}, | ||
[]bool{false, true, true}, | ||
2}, | ||
{"eventTypes as task", "task", | ||
[]proto.Message{taskRequest}, | ||
[]bool{true}, | ||
1}, | ||
{"eventTypes as node", "node", | ||
[]proto.Message{nodeRequest}, | ||
[]bool{true}, | ||
1}, | ||
{"eventTypes as workflow", "workflow", | ||
[]proto.Message{workflowRequest}, | ||
[]bool{true}, | ||
1}, | ||
{"eventTypes as workflow", "workflow", | ||
[]proto.Message{nodeRequest, taskRequest}, | ||
[]bool{false, false}, | ||
0}, | ||
{"eventTypes as task", "task", | ||
[]proto.Message{workflowRequest, nodeRequest}, | ||
[]bool{false, false}, | ||
0}, | ||
{"eventTypes as node", "node", | ||
[]proto.Message{workflowRequest, taskRequest}, | ||
[]bool{false, false}, | ||
0}, | ||
{"eventTypes as all", "all", | ||
[]proto.Message{workflowRequest, nodeRequest, taskRequest}, | ||
[]bool{true, true, true}, | ||
3}, | ||
{"eventTypes as *", "*", | ||
[]proto.Message{workflowRequest, nodeRequest, taskRequest}, | ||
[]bool{true, true, true}, | ||
3}, | ||
} | ||
for _, test := range tests { | ||
t.Run(test.name, func(t *testing.T) { | ||
initializeEventPublisher() | ||
var currentEventPublisher = NewEventsPublisher(mockEventPublisher, promutils.NewTestScope(), test.eventTypes) | ||
var cnt = 0 | ||
for id, event := range test.events { | ||
assert.Nil(t, currentEventPublisher.Publish(context.Background(), proto.MessageName(event), | ||
event)) | ||
if test.shouldSendEvent[id] { | ||
assert.Equal(t, proto.MessageName(event), testEventPublisher.Published[cnt].Key) | ||
marshalledData, err := proto.Marshal(event) | ||
assert.Nil(t, err) | ||
assert.Equal(t, marshalledData, testEventPublisher.Published[cnt].Body) | ||
cnt++ | ||
} | ||
} | ||
assert.Equal(t, test.expectedSendCnt, len(testEventPublisher.Published)) | ||
}) | ||
} | ||
} | ||
} | ||
|
||
func TestEventPublisher_PublishError(t *testing.T) { | ||
initializeEventPublisher() | ||
currentEventPublisher := NewEventsPublisher(mockEventPublisher, promutils.NewTestScope(), "*") | ||
var publishError = errors.New("publish() returns an error") | ||
testEventPublisher.GivenError = publishError | ||
assert.Equal(t, publishError, currentEventPublisher.Publish(context.Background(), | ||
proto.MessageName(taskRequest), taskRequest)) | ||
} |
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.
can we explicitly just use the
Type
of the Notificationsconfig. and if it's not no-op, error when Topic Name is empty?