Add page.on('metric')

[ankur22]

What?

This adds the functionality to group url tags by intercepting all metrics that the browser module emits, compare and match with the supplied regex(s), and then set the name and url tags with the new name supplied by the user in the script.

Why?

A lot of websites will perform requests that contain dynamic parts in the request url, which could be ids that are generated on every new session, account ids, or for cache busting purposes.

This will help in two ways:

1. Avoids high cardinality issues where tests work with websites with a lot of these dynamic urls.
2. It becomes trivial to correlate urls which are for the same page but contain dynamic parts in the url (ids/cache busting/etc).

How

The user first needs to register an event handler:

sequenceDiagram
  box main goroutine
  participant page.on('metric')
  participant p.eventHandlers
  end
  
  page.on('metric') ->> p.eventHandlers: add metric handler

Loading

This is a high level overview of what happens when a http request is started and a metric is about to be emitted:

sequenceDiagram
    box CDP handler goroutine
    participant emitRequestMetrics
    participant handleURLTag
    participant page.urlTagName
    end
    box taskqueue goroutine
    participant handler
    participant MetricEvent.Tag
    end
    box sobek context
    participant _k6BrowserURLGroupingTest
    end

    emitRequestMetrics ->> handleURLTag: url, tags
    handleURLTag ->> page.urlTagName: url
    page.urlTagName ->> page.urlTagName: create MetricEvent{url}
    loop each p.eventHandler
    page.urlTagName ->> handler: MetricEvent{url}
    Note right of handler: Regex supplied by user in handler in URLOverrides{}
    handler ->> MetricEvent.Tag: URLTagPatterns{}
    loop each URLTagPatterns
    MetricEvent.Tag ->> _k6BrowserURLGroupingTest: URLTagPattern.urlRegEx, url
    Note right of _k6BrowserURLGroupingTest: if match found, return true, else false
    _k6BrowserURLGroupingTest ->> MetricEvent.Tag: return true|false
    end
    Note left of MetricEvent.Tag: assign user defined name to MetricEvent.name if match found.
    MetricEvent.Tag ->> handler: return
    handler ->> page.urlTagName: return
    end
    Note left of page.urlTagName: read MetricEvent.name when it's not nil and set urlMatched.
    page.urlTagName ->> handleURLTag: return newTagName, urlMatched
    Note left of handleURLTag: set tags.url and tags.name as newTagName if urlMatched is true
    handleURLTag ->> emitRequestMetrics: return tags

Loading

The same happens when we're about to emit response and web vital metrics.

Checklist

• I have performed a self-review of my code
• I have added tests for my changes
• I have commented on my code, particularly in hard-to-understand areas

Related PR(s)/Issue(s)

Updates: #371

[inancgumus]

Nicely tackled with this complex issue 🙇! TBH, I should also admit that I'm not sure I understand this PR's code fully 😄 (my bad) So, I have some comments to discuss. None of them are strong opinions. Some suggestions are to hopefully make this change more descriptive, extendable, and maintainable.

[inancgumus]

Idea: If the generics solution doesn't turn out OK, we can also invent a new type, PageOnEvent, for Page.On events (eventHandlers map[string][]func(PageOnEvent)) to get rid of fragile runtime type assertions. PageOnEvent struct { ConsoleMessage *ConsoleMessage; ExportedMetric *ExportedMetric }. Each event can use only the field they are interested in, removing the need for type assertions.

[ankur22]

Idea: If the generics solution doesn't turn out OK, we can also invent a new type, PageOnEvent, for Page.On events (eventHandlers map[string][]func(PageOnEvent)) to get rid of fragile runtime type assertions. PageOnEvent struct { ConsoleMessage *ConsoleMessage; ExportedMetric *ExportedMetric }. Each event can use only the field they are interested in, removing the need for type assertions.

Nice alternative!

I've linked this comment to a comment in the generalize page.on events issue.

We can tackle the generalization after merging in this PR 👍

[ankur22]

Nicely tackled with this complex issue 🙇! TBH, I should also admit that I'm not sure I understand this PR's code fully 😄 (my bad) So, I have some comments to discuss. None of them are strong opinions. Some suggestions are to hopefully make this change more descriptive, extendable, and maintainable.

Thanks for the review 🙏 I agree that this is a bit confusing. I will create a diagram to make things a bit clearer.

[ankur22]

@inancgumus i created the diagram and placed it in the description of the issue. Let me know if it helps or not 🙂

[inancgumus]

Awesome work 🎉 I've also added some suggestions for you to consider before merging it in. These suggestions are highly likely due to my lack of understanding of this change enough and are not related to the code itself 😅

[inancgumus]

One last note.