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

Track DIPS gas metrics in csv #5215

Merged
merged 5 commits into from
Mar 20, 2024
Merged

Track DIPS gas metrics in csv #5215

merged 5 commits into from
Mar 20, 2024

Conversation

incrypto32
Copy link
Member

No description provided.

pub struct GasMetrics {
pub gas_counter: CounterVec,
pub op_counter: CounterVec,
pub gas_counter_map: Arc<RwLock<HashMap<String, u64>>>,
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if we should use something else instead of a RwLock

@incrypto32 incrypto32 force-pushed the krishna/gas-metrics-csv branch from 256520d to c8d46f8 Compare February 19, 2024 13:06
@incrypto32 incrypto32 self-assigned this Feb 19, 2024
@leoyvens leoyvens self-requested a review February 20, 2024 11:06
@leoyvens
Copy link
Collaborator

leoyvens commented Feb 22, 2024

Lets use object-store, instead of cloud-storage, it supports multiple cloud providers and very importantly supports local files.


/// Set by the env var `GRAPH_gas_metrics_gcs_bucket`
/// The name of the GCS bucket to store DIPS metrics
pub gas_metrics_object_store_url: Option<String>,
Copy link
Collaborator

@leoyvens leoyvens Mar 6, 2024

Choose a reason for hiding this comment

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

This URL is not just Cloud Storage anymore, now it also supports local file URLs. Worth documenting at least an example value for a local file.

graph/Cargo.toml Outdated
@@ -71,6 +71,9 @@ itertools = "0.12.0"
# Without the "arbitrary_precision" feature, we get the error `data did not match any variant of untagged enum Response`.
web3 = { git = "https://github.com/graphprotocol/rust-web3", branch = "graph-patches-onto-0.18", features = ["arbitrary_precision"] }
serde_plain = "1.0.2"
csv = "1.3.0"
object_store = { version = "0.9.1", features = ["gcp"] }
cloud-storage = { version = "0.11.1", features = ["global-client"] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets remove cloud-storage.

Copy link
Member Author

Choose a reason for hiding this comment

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

Noted, Yes right, still a work in progress, have some more local changes to be pushed

@incrypto32 incrypto32 marked this pull request as draft March 7, 2024 04:00
@incrypto32 incrypto32 force-pushed the krishna/gas-metrics-csv branch from 795bd4c to 25827fc Compare March 7, 2024 12:44
@incrypto32 incrypto32 marked this pull request as ready for review March 7, 2024 15:02
@incrypto32 incrypto32 requested a review from leoyvens March 11, 2024 07:02
@leoyvens
Copy link
Collaborator

What if we left GasMetrics as it was, since those Prometheus metrics may be useful for other purposes. And then added this new collector under the BlockState? From a Rust ownership perspective it may be nicer, we may be able to avoid the locks, and have flush_metrics_to_store consume self, such that fn reset_counters wouldn't be necessary.

@incrypto32 incrypto32 force-pushed the krishna/gas-metrics-csv branch from 25827fc to 4e35efd Compare March 14, 2024 15:34
@incrypto32 incrypto32 force-pushed the krishna/gas-metrics-csv branch from 0bf7f08 to da35c2d Compare March 18, 2024 16:49
Copy link
Collaborator

@leoyvens leoyvens left a comment

Choose a reason for hiding this comment

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

This code is very elegant now, great work! Just one comment.

}

#[derive(Debug)]
pub struct BlockStateMetrics {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd put this in a submodule in a separate file, just to keep things organized. But up to you.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@leoyvens
Copy link
Collaborator

I don't know if one file per block will be ok or not, but we can move forward with this and evaluate after testing a bit in real-life scenarios.

@incrypto32 incrypto32 merged commit 23763f7 into master Mar 20, 2024
7 checks passed
@incrypto32 incrypto32 deleted the krishna/gas-metrics-csv branch March 20, 2024 14:36
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.

2 participants