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

Store execTime metric in nanoseconds #5034

Closed
wants to merge 1 commit into from

Conversation

ssncferreira
Copy link
Contributor

@ssncferreira ssncferreira commented Jan 4, 2022

What this PR does / why we need it:

Enhancement: store execTime result summary statistic metric in nanoseconds.

Which issue(s) this PR fixes:
Fixes #4949 (comment)

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated
  • Add an entry in the CHANGELOG.md about the changes.

@ssncferreira ssncferreira changed the title Store metrics execTime in nanoseconds Store execTime metric in nanoseconds Jan 4, 2022
@ssncferreira ssncferreira marked this pull request as ready for review January 4, 2022 19:01
@ssncferreira ssncferreira requested review from KMiller-Grafana and a team as code owners January 4, 2022 19:01
@ssncferreira ssncferreira mentioned this pull request Jan 4, 2022
3 tasks
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

This LGTM, but I'd like to see if it'll affect Grafana's usage of this API

@@ -939,7 +939,7 @@ The example belows show all possible statistics returned with their respective d
},
"summary": {
"bytesProcessedPerSecond": 0, // Total of bytes processed per second
"execTime": 0, // Total execution time in seconds (float)
"execTime": 0, // Total execution time in nanoseconds (int)
Copy link
Member

Choose a reason for hiding this comment

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

@ivanahuckova Do you think this will negatively affect Grafana's usage of our statistics?

Copy link
Member

@ivanahuckova ivanahuckova Jan 5, 2022

Choose a reason for hiding this comment

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

Kind of. When we are processing result's statistic, we add a unit to the value, so we can then display it in nice way. E.g:

if it is more than 60 sec, we will show 1 minute:
image

if it is less than 1 sec, we will show ms
image

So if we change this, we will need to add logic to check if the value is in ns (integer) or sec(float) which will be slightly hacky, but possible. 🙂

So if you decide to change this, could you create issue in Grafana repo to change the logic there as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @ivanahuckova 🙂
As already discussed, both statistic time values execTime and queueTime will be returned by the HTTP API in second as float.

@ssncferreira ssncferreira force-pushed the metrics_exec_time_nanoseconds branch from 4932190 to 1aba0ac Compare January 5, 2022 11:05
@ssncferreira
Copy link
Contributor Author

Grafana uses the returned statistics from the HTTP API and expects time values to be in second as float. As discussed and in order to avoid subsequent changes to Grafana, this PR will be closed and the conversion of queueTime will be handled in PR #5052

@ssncferreira ssncferreira deleted the metrics_exec_time_nanoseconds branch January 5, 2022 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants