Add listeners to trace file metadata #571

jhrotko · 2024-02-12T14:57:36Z

Description

This PR intends to retrieve metadata from the parsing file. This was an issue when tracking the occurrences of extends in a project for telemetry since at the time the metrics are sent the Project is fully resolved thus no extends instances.

This will be one out of three PRs.
docker/compose#11492

Interaction

sequenceDiagram
    participant pinata
    participant compose
    participant compose-go
    compose->>compose-go: register listeners 
    compose->>+compose-go: load project
    compose-go->>+compose-go/extends: ApplyExtends
   Note right of compose-go/extends: options.ProcessEvent("extends", metadata)
    compose-go/extends-->>-compose-go: finish extends
    compose-go-->>-compose: finish load
    compose-->>pinata: send OTEL metrics

Fix

by adding the listener to increment the number of processed extends, found a bug where when going down the search tree, we were not updating the leafs after the merge. In a case where we had chained extends, we would over look the tree, since we would only update the services at the head of the recursion in ApplyExtends

ndeloof

Some design suggestion for more flexibility by consumer

Request changes on Project: it should NOT own FileMetadata, leave use of parsing events to the consumer - here you want to count extends, but there's other scenario we want to list files being processed, or ... mode details TBD

ndeloof · 2024-02-13T15:59:01Z

types/project.go

@@ -58,6 +58,18 @@ type Project struct {
 	// DisabledServices track services which have been disable as profile is not active
 	DisabledServices Services `yaml:"-" json:"-"`
 	Profiles         []string `yaml:"-" json:"-"`
+
+	// Metadata for telemetry
+	FileMetada *FileMetada `yaml:"-" json:"-"`


as Listener allows to track events during parsing, Project should not need to host such a metadata attribute. Just let the consumer manage collecting metrics with it's own data structure

ndeloof · 2024-02-13T16:01:09Z

cli/options.go

+
+	// Callbacks to retrieve metadata information during parse defined before
+	// creating the project
+	Listeners map[string][]loader.Listener


This makes it a bit complicated to write a listener for more than one event type. Could declare Listeners []Listener as Listener is a func(event string, details any, it already receives the event type, and writing a listener then is just a question of a switch and can select any interesting event and ignore all others.

@ndeloof by writing the way you are suggesting, you are passing the map functionality inside every registered listener by writing in all further listeners with a switch. When in reality with the current solution we take advantage of the map data structure to handle this. Also, when processing 1 listener, with your recomendation, you will need to iterate over all listeners, when in reality you just care for a subset of them. I can see one advantage in having a []Listener instead of map[string][]Listener which is, in case you want to have a particular Listener that is triggered for all events

I think the other way: if I want to collect multiple events, with your design I'll have to register mutliple listeners for all the events I'm interested in, while with a generic one I have a native filtering capability using switch:

func listener(event string ...) { switch event { case "extends": countExtends++ case "include": countInclude++ case "env_file": countEnvFile++ }

Also, if you're interested by a single event type, we can offer a Filter:

cli.WithListener( cli.FilterEvent("extends", func(...)) )

by the way, Listener doesn't need to be a slice, could be a single reference, and we can offer a composition helper

cli.WithListener( cli.CombineListeners(func(...), func(...), func(...)) )

tl;dr keep the original design simple/naive and offer helpers to give more flexibility to the consumer

ndeloof · 2024-02-13T16:04:21Z

loader/extends.go

@@ -60,6 +60,8 @@ func applyServiceExtends(ctx context.Context, name string, services map[string]a
 	if !ok {
 		return s, nil
 	}
+	opts.ProcessEvent("extends", name)


in addition to service name, would be useful to track the file so we can detect all files involved in a compose model
Listener's metadata parameter could be declared a map[string]any so it is easy to pass more than just one attribute here, and add more as this evolves without impacting consumers

this was a leftover from a test :) I intended to pass the extends (the whole extends metadata from the service) I will change this. I left a any in case for some reason in the future someone does not want to necessarily pass a map[string]any. What do you think @ndeloof ?

the benefit of a map[string]any is that this allows to "add more" without impact on previously reported keys. Using any the consumer would need to adjust to any change that's made to the original struct

ndeloof · 2024-02-13T16:07:08Z

loader/extends_test.go

+		options.Listeners = map[string][]Listener{
+			"extends": {func(event string, metadata any) {
+				extendsCount++
+			}},


Suggested change

}},

options.Listeners = []Listeners{

func(event string, metadata map[string]any {

if event == "extends" {

count ++

}

}},

milas

Agree with Nicolas' suggestions - I think a generic listener that gets the event type + metadata is fine, consumers can ignore what they don't care about. Other than that and last cleanup though, lgtm!

ndeloof

LGTM

ndeloof · 2024-02-14T13:04:11Z

cli/options.go

+func (o *ProjectOptions) WithListener(listeners []loader.Listener) {
+	o.Listeners = listeners


nit: Better make this a appending function so it can be used multiple times

Suggested change

func (o *ProjectOptions) WithListener(listeners []loader.Listener) {

o.Listeners = listeners

func (o *ProjectOptions) WithListener(listener loader.Listener) {

o.Listeners = append(o.Listeners, listener)

6e4fb2b#diff-0953236283edf016dbef569cb026a05dba9df5e7934af11b82499e3bec8fe006R374

Signed-off-by: jhrotko <[email protected]>

glours · 2024-02-14T13:29:12Z

cli/options.go

@@ -480,6 +491,13 @@ func withConvertWindowsPaths(options *ProjectOptions) func(*loader.Options) {
 	}
 }

+// save listeners from ProjectOptions (compose) to loader.Options
+func withListener(options *ProjectOptions) func(*loader.Options) {


As there could be more than one listener to save

Suggested change

func withListener(options *ProjectOptions) func(*loader.Options) {

func withListeners(options *ProjectOptions) func(*loader.Options) {

indeed. I plan to add include metric support using this mechanism so will fix this at same time in a follow-up PR

jhrotko mentioned this pull request Feb 12, 2024

Use listener for file metadata docker/compose#11492

Merged

jhrotko force-pushed the process-event-loader branch 4 times, most recently from 9bafbd8 to 05698d5 Compare February 13, 2024 15:24

jhrotko marked this pull request as ready for review February 13, 2024 15:24

jhrotko requested a review from ndeloof as a code owner February 13, 2024 15:24

jhrotko requested review from glours, milas and laurazard February 13, 2024 15:24

ndeloof requested changes Feb 13, 2024

View reviewed changes

milas reviewed Feb 13, 2024

View reviewed changes

jhrotko force-pushed the process-event-loader branch from 05698d5 to a6e592d Compare February 14, 2024 12:14

ndeloof approved these changes Feb 14, 2024

View reviewed changes

jhrotko force-pushed the process-event-loader branch from a6e592d to 0411918 Compare February 14, 2024 13:18

Add listeners to trace file metadata

6e4fb2b

Signed-off-by: jhrotko <[email protected]>

jhrotko force-pushed the process-event-loader branch from 0411918 to 6e4fb2b Compare February 14, 2024 13:24

glours reviewed Feb 14, 2024

View reviewed changes

ndeloof merged commit 26a5dd3 into compose-spec:main Feb 14, 2024
8 checks passed

jhrotko mentioned this pull request Feb 19, 2024

Fix env file read from include #582

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add listeners to trace file metadata #571

Add listeners to trace file metadata #571

jhrotko commented Feb 12, 2024 •

edited

Loading

ndeloof left a comment

ndeloof Feb 13, 2024

ndeloof Feb 13, 2024

jhrotko Feb 13, 2024 •

edited

Loading

ndeloof Feb 13, 2024

ndeloof Feb 13, 2024

jhrotko Feb 13, 2024

ndeloof Feb 13, 2024

ndeloof Feb 13, 2024

milas left a comment

ndeloof left a comment

ndeloof Feb 14, 2024

jhrotko Feb 14, 2024

glours Feb 14, 2024

ndeloof Feb 14, 2024

-			}},
+                  options.Listeners = []Listeners{
+                    func(event string, metadata map[string]any {
+                       if event == "extends" {
+                              count ++
+                       }
+			}},

		func (o *ProjectOptions) WithListener(listeners []loader.Listener) {
		o.Listeners = listeners

	func withListener(options ProjectOptions) func(loader.Options) {
	func withListeners(options ProjectOptions) func(loader.Options) {

Add listeners to trace file metadata #571

Add listeners to trace file metadata #571

Conversation

jhrotko commented Feb 12, 2024 • edited Loading

Description

Interaction

Fix

ndeloof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhrotko Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milas left a comment

Choose a reason for hiding this comment

ndeloof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhrotko commented Feb 12, 2024 •

edited

Loading

jhrotko Feb 13, 2024 •

edited

Loading