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: use millis in RPC metrics #4634

Merged
merged 2 commits into from
Aug 10, 2024
Merged

Conversation

LesnyRumcajs
Copy link
Member

@LesnyRumcajs LesnyRumcajs commented Aug 9, 2024

Summary of changes

Changes introduced in this pull request:

Based on the feedback from Glif:

  • changed the unit of RPC metrics to milliseconds, same as is reported in Lotus. This will facilitate cross-implementation performance testing,
  • added unit of measure to the metric description.

Based on my subjective opinion (and so I'm open to reverting that):

  • changed the histogram characteristics a bit, otherwise the majority of the methods would fall into all buckets. Should we change the bucket count to 5 and increase the factor to 10 @lemmih ?

Reference issue to close (if applicable)

Closes

Other information and links

# HELP rpc_processing_time Duration of RPC method call in milliseconds.
# TYPE rpc_processing_time histogram
rpc_processing_time_sum{method="Filecoin.ChainGetTipSetByHeight"} 0.039538
rpc_processing_time_count{method="Filecoin.ChainGetTipSetByHeight"} 1
rpc_processing_time_bucket{le="0.1",method="Filecoin.ChainGetTipSetByHeight"} 1
rpc_processing_time_bucket{le="1.0",method="Filecoin.ChainGetTipSetByHeight"} 1
rpc_processing_time_bucket{le="10.0",method="Filecoin.ChainGetTipSetByHeight"} 1
rpc_processing_time_bucket{le="100.0",method="Filecoin.ChainGetTipSetByHeight"} 1
rpc_processing_time_bucket{le="1000.0",method="Filecoin.ChainGetTipSetByHeight"} 1
rpc_processing_time_bucket{le="+Inf",method="Filecoin.ChainGetTipSetByHeight"} 1
rpc_processing_time_sum{method="Filecoin.ChainExport"} 35033.210394999995
rpc_processing_time_count{method="Filecoin.ChainExport"} 1
rpc_processing_time_bucket{le="0.1",method="Filecoin.ChainExport"} 0
rpc_processing_time_bucket{le="1.0",method="Filecoin.ChainExport"} 0
rpc_processing_time_bucket{le="10.0",method="Filecoin.ChainExport"} 0
rpc_processing_time_bucket{le="100.0",method="Filecoin.ChainExport"} 0
rpc_processing_time_bucket{le="1000.0",method="Filecoin.ChainExport"} 0
rpc_processing_time_bucket{le="+Inf",method="Filecoin.ChainExport"} 1
rpc_processing_time_sum{method="Filecoin.StateNetworkName"} 0.43903800000000006
rpc_processing_time_count{method="Filecoin.StateNetworkName"} 5
rpc_processing_time_bucket{le="0.1",method="Filecoin.StateNetworkName"} 3
rpc_processing_time_bucket{le="1.0",method="Filecoin.StateNetworkName"} 5
rpc_processing_time_bucket{le="10.0",method="Filecoin.StateNetworkName"} 5
rpc_processing_time_bucket{le="100.0",method="Filecoin.StateNetworkName"} 5
rpc_processing_time_bucket{le="1000.0",method="Filecoin.StateNetworkName"} 5
rpc_processing_time_bucket{le="+Inf",method="Filecoin.StateNetworkName"} 5
rpc_processing_time_sum{method="Filecoin.ChainHead"} 0.058253
rpc_processing_time_count{method="Filecoin.ChainHead"} 1
rpc_processing_time_bucket{le="0.1",method="Filecoin.ChainHead"} 1
rpc_processing_time_bucket{le="1.0",method="Filecoin.ChainHead"} 1
rpc_processing_time_bucket{le="10.0",method="Filecoin.ChainHead"} 1
rpc_processing_time_bucket{le="100.0",method="Filecoin.ChainHead"} 1
rpc_processing_time_bucket{le="1000.0",method="Filecoin.ChainHead"} 1
rpc_processing_time_bucket{le="+Inf",method="Filecoin.ChainHead"} 1

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

@LesnyRumcajs LesnyRumcajs requested a review from a team as a code owner August 9, 2024 12:52
@LesnyRumcajs LesnyRumcajs requested review from ruseinov, hanabi1224 and lemmih and removed request for a team August 9, 2024 12:52
@lemmih
Copy link
Contributor

lemmih commented Aug 9, 2024

changed the histogram characteristics a bit, otherwise the majority of the methods would fall into all buckets. Should we change the bucket count to 5 and increase the factor to 10 @lemmih ?

That sounds great to me.

@@ -44,7 +44,8 @@ where

metrics::RPC_METHOD_TIME
.get_or_create(&method)
.observe(start_time.elapsed().as_secs_f64());
// Observe the elapsed time in milliseconds
.observe(start_time.elapsed().as_secs_f64() * 1000.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Too bad .as_millis_f64() isn't stable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that is disappointing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could still use a millisecond constant from Duration instead of the magic number.

Suggested change
.observe(start_time.elapsed().as_secs_f64() * 1000.0);
.observe(start_time.elapsed().as_secs_f64() * Duration::MILLISECOND as f64);

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we couldn't. That constant is nightly-only. https://doc.rust-lang.org/std/time/struct.Duration.html#associatedconstant.MILLISECOND

Even if it wasn't nightly, the suggested code would still not work. Conceptually, you'd convert, e.g., 3s to 3ms.

We could introduce a named constant, but given it's used exclusively here and it's pretty trivial, combined with the comment above, I'm not convinced it'd bring any value.

Do you agree?

Copy link
Contributor

@ruseinov ruseinov Aug 10, 2024

Choose a reason for hiding this comment

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

Oops, yeah, this was a bit rushed. Not a blocker, however still not a fan of magic numbers. IMHO it's best to introduce a self-documenting constant than a comment, both are exactly one line of code.

@ruseinov
Copy link
Contributor

ruseinov commented Aug 9, 2024

Looks good to me, but would like to see the 1000.0 magic number gone.

@LesnyRumcajs LesnyRumcajs added this pull request to the merge queue Aug 10, 2024
Merged via the queue into main with commit a5fb8c3 Aug 10, 2024
30 checks passed
@LesnyRumcajs LesnyRumcajs deleted the use-millis-in-rpc-metrics branch August 10, 2024 13:20
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