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

feat: add request statistics #261

Merged
merged 5 commits into from
Nov 28, 2023
Merged

feat: add request statistics #261

merged 5 commits into from
Nov 28, 2023

Conversation

Rasmus-Bertell
Copy link
Contributor

This PR adds the option to configure statistics for requests with the curl --write-out option.

Statistics are added after the headers but before the body, and are by default disabled so this is an opt in feature and won't affect any existing configurations.

There are a couple of ways to configure statistics, either with a string or with a statistics spec. I didn't know how to properly convey this in the README.md, so any suggestions are welcome.

The two different config options are shown below and can be mixed together also.

{
  result = {
    show_statistics = { "time_total", "size_download" }
  }
}
{
  result = {
    show_statistics = {
      { "time_total", title = "Total time: ", type = "time" },
      { "size_download", type = "byte" },
  }
}

They will output respectively (queries ran on different endpoints, so their output doesn't match)

time_total 0.189020
size_download 154
Total time: 323.24 ms
size_download 6.96 KiB

depends #260
closes #256

Any feedback is welcome and I'll be happy to hear how I can improve my code.

@NTBBloodbath
Copy link
Member

Hey bud, hope you're doing well!

Right now it's my time to sleep but I'll try to make some time for myself tomorrow morning. I will review #260 first since these changes depend on those and then I will come in more detail to this if that's okay.

A quick question that may be unnecessary after I do the review, but what data types are used by default in the statistics output? As I can see in the most complex example it was changed to milliseconds and bytes for the time and download size respectively and I wonder why it is not the default value, at least for the size to improve readability since the time taken I assume is in seconds by default :p

Cheers

@Rasmus-Bertell
Copy link
Contributor Author

I opted to have the output as is by default and not apply any transformations, since this PR allows the user to use any of curls --write-out params and having default types for some fields didn't make sense at the time I was writing this code.

However I can see how it would be beneficial to have some default transformations for example variables starting with time_* or size_* would be transformed to seconds or bytes respectively.

I'll adjust the code today sometime after work to have the default types.

@NTBBloodbath
Copy link
Member

Cool!

Yeah, I feel like having some fields with "healthy" values ​​would improve the ootb experience a bit for someone who enables the stats option. Yesterday I was finishing my blog and I lost track of time fighting with CSS, my apologies. As soon as I have my morning coffee I will start the review of the previous PR and wait for the new changes here 😄

@Rasmus-Bertell
Copy link
Contributor Author

Took a bit longer until I got the motivation to do the changes but I pushed the latest changes just now. It seems, however, that github is a bit broken, you'll have to wait until https://www.githubstatus.com/incidents/66vhjmd266r9 is fixed before reviewing.

@Rasmus-Bertell
Copy link
Contributor Author

And immediately after commenting the changes showed up here, typical.

@NTBBloodbath
Copy link
Member

Took a bit longer until I got the motivation to do the changes but I pushed the latest changes just now.

Haha it do be like that sometimes, no worries :p

It seems, however, that github is a bit broken, you'll have to wait until githubstatus.com/incidents/66vhjmd266r9 is fixed before reviewing.

Woops, unlucky. It seems to have been fixed now, so I'll take a look at it :)

@NTBBloodbath
Copy link
Member

I had some unforeseen things to do yesterday, I couldn't finish approving the changes haha ​​what bad luck. Everything looks very good to me, thank you very much for the contribution!

@NTBBloodbath NTBBloodbath merged commit 89c02d1 into rest-nvim:main Nov 28, 2023
2 checks passed
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.

Add statistics for requests
2 participants