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

Fuel-Executor Metrics #807

Closed
4 tasks done
ControlCplusControlV opened this issue Dec 1, 2022 · 7 comments
Closed
4 tasks done

Fuel-Executor Metrics #807

ControlCplusControlV opened this issue Dec 1, 2022 · 7 comments

Comments

@ControlCplusControlV
Copy link
Contributor

ControlCplusControlV commented Dec 1, 2022

Scope changes:

The following metrics will be added

  • histogram: gas per block
  • histogram: size per block
  • histogram: fee/coinbase per block
  • histogram: txs per block

The following metric will not be added as a part of this issue. We will discuss if we want some more sophisticated mechanism to measure other operations that touch DB

  • histogram: I/O (db) time per block

The following metric will not be added

cc: @acerone85 @MitchTurner

@rafal-ch
Copy link
Contributor

rafal-ch commented Oct 7, 2024

Question regarding "histogram: I/O (db) time per block" - what we actually try to measure here?

Are we interested in how much time was spent on the DB operations in Importer::execute_and_commit?

This would require measuring call-chain such as:

  • verify_block_fields()
    • block_header_merkle_root()
    • block_header()
  • validate()
    • native_validate_inner() -or- wasm_validate_inner()
    • potential call to get_module() in validate_inner()
  • _commit_result()
  • possibly more...

And then, next to

importer_metrics().execute_and_commit_duration.observe(time);

we can add

importer_metrics().execute_and_commit_db_duration.observe(time);

Alternatively, we might want to work on a generic method of opt-in for measured DB reads, like trait MeasuredStorageInspect or similar.


Any opinions? @xgreenx @acerone85

@acerone85
Copy link
Contributor

The gas price is not available from the block importer, but rather from the producer.
Should we add metrics for the block producer too @MitchTurner?

@rafal-ch
Copy link
Contributor

rafal-ch commented Oct 7, 2024

Also, what would be the suggested values for FEE_BUCKETS and GAS_PRICE_BUCKETS buckets.

@rymnc
Copy link
Member

rymnc commented Oct 7, 2024

Also, what would be the suggested values for FEE_BUCKETS and GAS_PRICE_BUCKETS buckets.

cc: @MitchTurner

@MitchTurner
Copy link
Member

The gas price is not available from the block importer, but rather from the producer.
Should we add metrics for the block producer too @MitchTurner?

Perhaps we put the metrics on the gas-price-service since it holds the most context? It's available from the producer, the tx-pool, and graphql, but they all get it served from the gas price service.

@rafal-ch
Copy link
Contributor

rafal-ch commented Oct 8, 2024

As for now, we're not gonna be doing the "gas price" metric. Rationale: Since we don't have DA gas price yet, we can derive the total gas price from gas used and fee.

xgreenx added a commit that referenced this issue Oct 14, 2024
Closes #807

## Description
This PR adds a couple of additional metrics (block importer and p2p) and
also contains slight refactor of how we initialize bucket sized for
histogram based metrics.

The `metrics` command-line parameter has been replaced with
`disable-metrics`. Metrics are now enabled by default, with the option
to disable them entirely or on a per-module basis. This change is
_breaking_ for all dependencies that use CLI to setup the
`fuel-core-client`

```
      --disable-metrics <METRICS>
          Disables all metrics, or specify a comma-separated list of modules to disable metrics for specific ones. Available options: importer, p2p, producer, txpool, graphql
          
          [env: METRICS=]
          [default: ]
```

Startup logs also show the metrics config:
```
2024-10-14T20:17:37.536840Z  INFO fuel_core_bin::cli::run: 308: Metrics config: Disable modules: txpool
```

## Checklist
- [X] Breaking changes are clearly marked as such in the PR description
and changelog
- [X] New behavior is reflected in tests

### Before requesting review
- [X] I have reviewed the code myself

---------

Co-authored-by: acerone85 <andrea.cerone@gmail.com>
Co-authored-by: rymnc <43716372+rymnc@users.noreply.github.com>
Co-authored-by: Green Baneling <XgreenX9999@gmail.com>
@MitchTurner
Copy link
Member

Everything looks completed :)

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

No branches or pull requests

5 participants