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

Update CreateDashboard Script #26

Merged
merged 10 commits into from
Feb 21, 2024
Merged

Conversation

ardelato
Copy link
Contributor

Description

There have been quite a few changes to the underlying core logic. This has affected the metrics being sent to DataDog which in turn means the dashboards created through the createDashboard script are now outdated and ineffective.

The createDashboard script has been revised in this update to enhance the efficiency and functionality of the dashboards it generates.

Before After
image image

CR

This mainly adds a new Query Value widget to the dashboard building logic. The new Query Value widget provides an average for all URLs of each page type, facilitating quick issue identification and correlation with data points. In contrast, the Timeseries widget is more useful at tracking changes over time for each URL. Additionally, the
Timeseries widget has been enhanced to easily incorporate customLinks, allowing for direct access to relevant pages associated with the data points.

QA

  • Confirm running pnpm create-dashboard now creates a dashboard that shows the data properly
  • Confirm for the Timeseries widget the datapoints have a Visit Webpage link that will take you to the actual webpage that was audited.

Closes: https://github.com/iFixit/ifixit/issues/51901

We have added the feature of being able to run `vigilo` in a Kubernetes
cluster which means the host tag sent to DataDog will be 'docker-container'.
Therefore, let's use an environment variable to configure the host query
parameter.
We will be adding a Query Value widget to the dashboard so we will need to
add alerts to said widget. Let's differentiate between the Timeseries
and Query Value alerts and add the new Query Value alerts
Similar to the timeseries request function, we will create a new function to
handle the creation of requests for the query value widget. One thing to note,
unlike the timeseries widget, the query value widget implements the alerts
through the requests definition.
We've updated the lighthouse metrics to send both a 'score' and 'value' datapoint
for each audit. As such we need to update the queries to specify the new
subpath. For most of the audits, the 'value' datapoint is more useful than the
'score' datapoint. The only audits that we would use the 'score' datapoint are
those with null 'value' datapoints like the 'is_crawlable' audit.
Similar to the Timeseries widget function, this adds a new function to create
a Query Value widget.
We will need to pair up the query value widget and timeseries widget together
in the same row. On top of that, we don't want the widgets to expand the full
width. Therefore, we will pass a parameter to modify the size and position
of the widgets.
This modifies the createWidgetForAllPageTypes function to create a query value
widget at the same time as the timeseries widget. On top of that, we will
keep track of the position of the widgets to layout them out properly per
group.
This adds a custom link to the timeseries widgets such that we can link the
actual webpage for the datapoint being displayed. This will make it convenient
to debug and investigate the data without having to manually type out the URL.
@ghost
Copy link

ghost commented Feb 14, 2024

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@deltuh-vee deltuh-vee self-assigned this Feb 14, 2024
@deltuh-vee deltuh-vee added the QAing Under QA team review label Feb 14, 2024
@deltuh-vee
Copy link
Contributor

QA 🎬

  • Running pnpm create-dashboard now creates a dashboard that shows the data properly.
  • For the Timeseries widget, the datapoints have a Visit Webpage link that will take you to the actual webpage that was audited.

@deltuh-vee deltuh-vee removed the QAing Under QA team review label Feb 14, 2024
For some of the audits, we weren't scaling the comparator value to the correct
unit types of the metrics being sent. Most of the metrics are in milliseconds,
so we need to scale the comparator value to milliseconds as well as these values
assumed the metric was in seconds.
The alert display type is CamelCase, not snake_case. This caused the warning
marker to not have the correct display type.
@kthaler kthaler self-assigned this Feb 15, 2024
@kthaler kthaler added the QAing Under QA team review label Feb 15, 2024
@kthaler
Copy link

kthaler commented Feb 15, 2024

QA 💎 - Running pnpm create-dashboard creates a dashboard with the same format as the one in the PR description.

deploy_block 🦖 - I am not seeing the "Visit Webpage" link on the datapoints in a Timeseries widget:

Screenshot 2024-02-14 at 5 36 10 PM

@ardelato
Copy link
Contributor Author

un_deploy_block ⚡
@kthaler was that graph manually added? If not, what does your lh-config.js and urls.json files look like?

The other graphs within the groups do have the Visit Webpage links.

image

@kthaler kthaler added QAing Under QA team review and removed QAing Under QA team review labels Feb 15, 2024
@kthaler
Copy link

kthaler commented Feb 15, 2024

un_deploy_block 🦕
QA 💎 - The data points on the Timeseries widget have a link to the webpage that was audited.

@kthaler kthaler removed the QAing Under QA team review label Feb 15, 2024
Copy link
Member

@mlahargou mlahargou left a comment

Choose a reason for hiding this comment

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

I don't pretend to understand all of the queries. I think QA is more important there.

The code seems sane. CR 📱

"display_type": "warning dashed"
}
] : []
}

function fetchQueryValueAlertMarkersForAudit(auditName: string) {
return ALERT_MARKERS_QUERY_VALUE.hasOwnProperty(auditName) ? ALERT_MARKERS_QUERY_VALUE[auditName] : []
Copy link
Member

Choose a reason for hiding this comment

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

I see us using hasOwnProperty in a few places. Can't we simplify this?

return ALERT_MARKERS_QUERY_VALUE[auditName] ?? []

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH, this whole script could be improved and simplified. I thought about refactoring it by using a Factory design pattern but that is quite the over haul for the scope of the issue.

main...testing-factory-pattern-for-create-dashboards-script

@ardelato ardelato merged commit 617902e into main Feb 21, 2024
1 check passed
@ardelato ardelato deleted the update-create-dashboards-script branch February 21, 2024 18:58
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.

4 participants