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

Assorted remote metrics fixes. #15914

Merged
merged 2 commits into from
Jun 24, 2022

Conversation

stuhood
Copy link
Member

@stuhood stuhood commented Jun 23, 2022

  • Fix the RemoteStoreReadBlobTimeMicros metric, which was not including the actual fetch time.
  • Add the relevant unit suffix to time-based metrics which don't have one.
  • Move RemoteStoreBlobBytes{Up,Down}loaded to observation metrics, in order to record histograms.
    • Additionally, report a sum value for histograms.
  • Simplify the "time to first byte" calculation by using an Option<Instant> for the start time.

[ci skip-build-wheels]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@stuhood stuhood added needs-cherrypick category:bugfix Bug fixes for released features labels Jun 23, 2022
@stuhood stuhood added this to the 2.13.x milestone Jun 23, 2022
@stuhood stuhood enabled auto-merge (squash) June 23, 2022 23:45
@stuhood stuhood disabled auto-merge June 23, 2022 23:56
# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@stuhood
Copy link
Member Author

stuhood commented Jun 24, 2022

Added one more commit on top to render the sum of histogram values (to make up for the loss of RemoteStoreBlobBytes{Down,Up}loaded as precomputed values).

@stuhood stuhood merged commit b071ebe into pantsbuild:main Jun 24, 2022
@stuhood stuhood deleted the stuhood/remote-store-metrics-fixes branch June 24, 2022 18:58
stuhood added a commit to stuhood/pants that referenced this pull request Jun 28, 2022
* Fix the `RemoteStoreReadBlobTimeMicros` metric, which was not including the actual fetch time.
* Add the relevant unit suffix to time-based metrics which don't have one.
* Move `RemoteStoreBlobBytes{Up,Down}loaded` to observation metrics, in order to record histograms.
    * Additionally, report a `sum` value for histograms.
* Simplify the "time to first byte" calculation by using an `Option<Instant>` for the start time.

[ci skip-build-wheels]
stuhood added a commit that referenced this pull request Jun 28, 2022
* Fix the `RemoteStoreReadBlobTimeMicros` metric, which was not including the actual fetch time.
* Add the relevant unit suffix to time-based metrics which don't have one.
* Move `RemoteStoreBlobBytes{Up,Down}loaded` to observation metrics, in order to record histograms.
    * Additionally, report a `sum` value for histograms.
* Simplify the "time to first byte" calculation by using an `Option<Instant>` for the start time.

[ci skip-build-wheels]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:bugfix Bug fixes for released features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants