Fix observability

[jt-dd]

Reworking the observability stack to get metrics, logs and events from all differentes phases of processing.

[edznux-dd]

2 comments (I may miss understanding something):

• GetRunIDFromContext feels weird to be in the log package?
• Can't we "automatically" do that in the custom span package directly ? Since you alread set default tags in there ? This would also simplify adding other custom field?

It would simplify that call in every places to only the 2 lines:

spanPut, outer = span.SpanRunFromContext(outer, span.IngestorBlobPull)
defer func() { spanPut.Finish(tracer.WithError(err)) }()

[jt-dd]

This is a special usecase for the blob. Here it is not to span too many spans (adding a comment).

Where would you prefer GetRunIDFromContext() to be ?

[edznux-dd]

I believe it would be a nice improvement to use these as a custom EventAction type instead of string.

This way, you can enforce the strings to match your expectation.
The defined api contract is a bit "weird" here imo, as it contains package specific code (PushEventIngestSkip is tied to the Ingestor, directly in its name). I'd not mind if it was is a struct / map "as data" for collocation purpose, but as an exposed API it's couple the 2 packages.

You could refactor all of these individual function into a single one like so (following code not tested) to avoid the duplication for each event and makes it simpler and easier to udpate overall:

type EventAction string

const (
	EventActionFail   = "fail"
	EventActionInit   = "init"
	EventActionStart  = "start"
	EventActionSkip   = "skip"
	EventActionFinish = "finish"
)

func (ea EventAction) Message() string{
	return map2msg[ea]
}

func (ea EventAction) Tags() []string {
	tags := tag.GetDefaultTags(ctx)
	tags = append(tags, fmt.Sprintf("%s:%s", ActionTypeTag, ea))
	return tags 
}
func (ea EventAction) Level() statsd.Level {
	return statsd.Info
}

// Could also be a format stirng template in this case, if needed?
var map2msg = map[EventAction]string{
	EventActionFail = "Ingestor/grpc endpoint init failed",
	EventActionInit = "" //...
	// ...
}

func PushEvent(ctx context.Context, action EventAction) error{
	return kstatsd.Event(&statsd.Event{
		Title:     action.Title(),
		Text:      action.Message(),
		Tags:      action.Tags(),
		AlertType: action.Level(),
	})
}

[jt-dd]

ok make sense but some events requires args that needs to be passed. So they will need a custom PushEvent wrapper for those specific.

[edznux-dd]

I'd think an opt Option or opt ...Option argument (variadic) type would be enough and allow basic definition to not need it (for the variadic version)? But I haven't tried.

(the variadic version has the edge case of multiple option provided, which is a bit more annoying to implement and is probably not worth the hassle here?)

[edznux-dd]

Not a fan of having the "exposed" API with individual actions instead of just calling pushEvent() directly, but 🤷

[jt-dd]

Not a fan of having the "exposed" API with individual actions instead of just calling pushEvent() directly, but 🤷

As discussed refactor with PushEvent() only exposed.