Skip to content
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

Move the name field on the `page.on('metric') API #1489

Merged
merged 6 commits into from
Oct 17, 2024

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented Oct 17, 2024

What?

This change does two things:

  1. Moves the name field to the level above.
  2. Rename the urls field to matches.

So from:

  page.on('metric', (metric) => {
    metric.tag({
      urls: [
        {url: /^https:\/\/test\.k6\.io\/\?q=[0-9a-z]+$/, name:'test'},
      ]
    });
  });

to:

  page.on('metric', (metric) => {
    metric.tag({
      name:'test',
      matches: [
        {url: /^https:\/\/test\.k6\.io\/\?q=[0-9a-z]+$/},
      ]
    });
  });

Why?

It was unclear what name was doing when it was part of the "match" object and it made it difficult to add other fields to the object to match against.

urls doesn't lend itself to extending with more fields to match against. It also doesn't represent what it is doing, whereas matches is a helpful verb that makes it clearer to the user/reader of the API what its impact is.

By moving the name field one level up, and renaming urls to matches, we can now read this API:

    metric.tag({
      name:'test',
      matches: [
        {url: /^https:\/\/test\.k6\.io\/\?q=[0-9a-z]+$/},
      ]
    });

as:

  1. tag metric;
  2. with this name;
  3. that matches.

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: #1487

This moves the name field into the level above. This makes the API less
confusing and allows us to extend the API to add other fields apart
from just urls.

We can now read the API as:

* metric.tag
* with name
* when urls match
This is a better representation of what we're doing with the fields.
They are being used to find matches in the existing metric tags that
are about to be emitted.
Since the name field is now one level above, it doesn't need to be in
the for loop.
Renaming userProvidedTagName to userProvidedURLTagName since it's
important to know that the name tag will be used on the url tag.
@ankur22 ankur22 requested a review from inancgumus October 17, 2024 10:32
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@ankur22 ankur22 merged commit bb53cc6 into main Oct 17, 2024
23 checks passed
@ankur22 ankur22 deleted the update/page-on-metric-api branch October 17, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants