-
Notifications
You must be signed in to change notification settings - Fork 223
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
Add page.on('metric') docs #1807
Conversation
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.
LGTM 🚀 Some suggestions.
docs/sources/k6/next/javascript-api/k6-browser/metricmessage.md
Outdated
Show resolved
Hide resolved
docs/sources/k6/next/javascript-api/k6-browser/metricmessage.md
Outdated
Show resolved
Hide resolved
docs/sources/k6/next/javascript-api/k6-browser/metricmessage.md
Outdated
Show resolved
Hide resolved
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.
Left a few comments with some small edits. I think the biggest thing might be tweaking the descriptions of the properties just a bit. 🤓
@@ -0,0 +1,60 @@ | |||
--- | |||
title: 'MetricMessage' | |||
slug: 'metricmessage' |
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.
slug: 'metricmessage' |
If the URL is going to be the same name as the filename, you don't need the slug
property here. :)
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.
Sure, removed in 3baf386
docs/sources/k6/next/javascript-api/k6-browser/metricmessage.md
Outdated
Show resolved
Hide resolved
docs/sources/k6/next/javascript-api/k6-browser/metricmessage.md
Outdated
Show resolved
Hide resolved
docs/sources/k6/next/javascript-api/k6-browser/metricmessage.md
Outdated
Show resolved
Hide resolved
|
||
| Parameter | Type | Description | | ||
| ----------------------- | -------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| tagMatch | object | It is required. | |
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 include something more descriptive here? Maybe "The tagMatch object and its properties that are used for matching metrics. Required."
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.
Sure, added in d1f8377
| Parameter | Type | Description | | ||
| ----------------------- | -------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| tagMatch | object | It is required. | | ||
| tagMatch.name | string | It is a required field and it must not be an empty string. It will replace the current metric's url and name tag values when a match is found. | |
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.
I'm a little bit confused by this description and the following ones as well. 🤔
What happens with the value of this property? I'm wondering if we could tweak the description for this and the following values to be something along the lines of: "The name value that replaces the current metric's URL and name tag values, if a match is found. Required, and must not be an empty string."
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.
I've made the change, WDYT? 152d0b3
Co-authored-by: Heitor Tashiro Sergent <[email protected]>
@heitortsergent i've merged this PR in for now. Any new feedback I will work on in a new PR. 🙇 |
What?
This adds the docs for
page.on('metric')
, which is a new API in the browser module.Checklist
npm start
command locally and verified that the changes look good.docs/sources/next
folder of the documentation.Related PR(s)/Issue(s)
grafana/xk6-browser#371, grafana/xk6-browser#1487