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

Restructure metrics #1151

Merged
merged 17 commits into from
May 4, 2023
Merged

Restructure metrics #1151

merged 17 commits into from
May 4, 2023

Conversation

MattDodsonEnglish
Copy link
Contributor

@MattDodsonEnglish MattDodsonEnglish commented Apr 26, 2023

Possible improvements:

@github-actions
Copy link
Contributor

There's a version of the docs published here:

https://mdr-ci.staging.k6.io/docs/refs/pull/1151/merge

It will be deleted automatically in 30 days.

@ppcano
Copy link
Collaborator

ppcano commented May 2, 2023

@MattDodsonEnglish Something to consider as part of these changes:

  • On the Metrics reference page, include metrics from experimental modules such as new websockets, redis, browser... This will give more visibility to these modules.

  • Add a section on HTTP API, WS API, gRPC API ... that list the collected metrics for that module or link to its section on the Metrics reference page.

@MattDodsonEnglish
Copy link
Contributor Author

@ppcano Perhaps those can happen in a separate PR? I'd like to merge this, then iterate on top. As it stands, I think even merging today is an improvement, so I'd rather do it incrementally.

Also, some comments

  • On the Metrics reference page, include metrics from experimental modules such as new websockets, redis, browser... This will give more visibility to these modules.

It looks like the docs for redis and maybe browser don't have metrics yet. Need to investigate what they are.

  • Add a section on HTTP API, WS API, gRPC API ... that list the collected metrics for that module or link to its section on the Metrics reference page.

I agree, I'm not sure what strategy is better:

  • Duplicate the metrics. If so, it probably would be better to make a shared component to ensure that both versions stay accurate.
  • Just link to the metrics. A little less convenient, perhaps, but simpler to write and less text to read.

@MattDodsonEnglish MattDodsonEnglish requested a review from ppcano May 2, 2023 14:28
Comment on lines +12 to +13
If you need help constructing a custom metric, read the following sections of this document.
They document the procedure and provide examples.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we adding examples for each type in this page? Isn't it better to have them in the dedicated doc for each type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do also have examples in the metrics reference docs. It is indeed redundant, but I'm trying to think of how someone who's looking to make a custom metric may arrive in one of two places (here or the reference).

It's also at the end, so the most important information is at the top.

Not sure though, maybe it'd be better to just delete and remove the duplication... 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe one could be a link to the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's better, more efficient.

Here's a nice red diff: 7de40f7

MattDodsonEnglish and others added 2 commits May 2, 2023 12:14
Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>
Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>
@MattDodsonEnglish MattDodsonEnglish changed the title wip: restructure metrics Restructure metrics May 2, 2023
@MattDodsonEnglish MattDodsonEnglish requested a review from codebien May 2, 2023 16:18
It applies really to thresholds
@MattDodsonEnglish MattDodsonEnglish merged commit 90fe98f into main May 4, 2023
@MattDodsonEnglish MattDodsonEnglish deleted the 962/rewrite-metrics branch May 4, 2023 14:19
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.

3 participants