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.

Sign up for GitHub

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

Jump to bottom

Add logging of the long GraphQL queries for future debug #1250

Merged
merged 9 commits into from
Jul 14, 2023
Merged

Add logging of the long GraphQL queries for future debug #1250

merged 9 commits into from
Jul 14, 2023

Conversation

MitchTurner
Copy link
Member

#1186

This PR:

  • Creates new extension for logging
  • Adds new extension to GraphQL API
  • Allows caller of new_service to customize the log_threshold_ms for how long a query must take for it to get logged

@MitchTurner
Copy link
Member Author

I left a TODO on the actual logging because I wasn't sure if there was more details we wanted to include.

leviathanbeak
leviathanbeak reviewed Jul 13, 2023
View reviewed changes
Copy link
Contributor

@leviathanbeak leviathanbeak left a comment

Choose a reason for hiding this comment

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

looks good, will wait for removal of todo to approve

xgreenx
xgreenx reviewed Jul 13, 2023
View reviewed changes
Copy link
Collaborator

@xgreenx xgreenx left a comment

Choose a reason for hiding this comment

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

It would be nice if you tried to test it locally and put a screenshot with the output=)

You can start a node in debug mode and put a breakpoint somewhere in the endpoint to simulate long query=)

crates/fuel-core/src/service/sub_services.rs Outdated Show resolved Hide resolved
crates/fuel-core/src/graphql_api/logging.rs Outdated Show resolved Hide resolved
crates/fuel-core/src/graphql_api/logging.rs Outdated Show resolved Hide resolved
@MitchTurner
Copy link
Member Author

MitchTurner commented Jul 13, 2023 •
edited
Loading

image

It's a little noisy, I assume because the resolvers are recursive. But that's probably useful so we can identify which request in the recursion is the slow one.

@Voxelot
Copy link
Member

Voxelot commented Jul 13, 2023

It might be good to log this all in one line. Otherwise if there are a lot of concurrent requests the logs linking the query args could be out of order.

@MitchTurner
Copy link
Member Author

Need to rename prometheus.rs

@MitchTurner
Copy link
Member Author

What do we prefer?

Readable:
image

or compact:
image

@xgreenx
Copy link
Collaborator

xgreenx commented Jul 14, 2023

Readable looks nice and definitely is simpler to find in the terminal, but for elastic search, I think it is better to compact=)

@MitchTurner MitchTurner force-pushed the add-logging-of-long-graphql-queries branch from 469a437 to 8b6e9fe Compare July 14, 2023 16:58
xgreenx
xgreenx previously approved these changes Jul 14, 2023
View reviewed changes
crates/fuel-core/src/graphql_api/service.rs Outdated
@@ -161,6 +162,7 @@ pub fn new_service(
txpool: TxPool,
producer: BlockProducer,
consensus_module: ConsensusModule,
log_threshold_ms: Duration,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
log_threshold_ms: Duration,
_log_threshold_ms: Duration,

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't this syntax for vars that are unused? I'm missing why I'd change it here.

crates/fuel-core/src/graphql_api/service.rs Outdated
@@ -173,7 +175,7 @@ pub fn new_service(
let builder = builder.extension(async_graphql::extensions::Tracing);

#[cfg(feature = "metrics")]
let builder = builder.extension(PrometheusExtension {});
let builder = builder.extension(MetricsExtension::new(log_threshold_ms));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let builder = builder.extension(MetricsExtension::new(log_threshold_ms));
let builder = builder.extension(MetricsExtension::new(_log_threshold_ms));

@MitchTurner MitchTurner enabled auto-merge (squash) July 14, 2023 17:24
@MitchTurner MitchTurner merged commit 456114c into master Jul 14, 2023
@MitchTurner MitchTurner deleted the add-logging-of-long-graphql-queries branch July 14, 2023 17:53
This was referenced Jul 16, 2023
Merged
Closed
xgreenx added a commit that referenced this pull request Jul 17, 2023
@xgreenx
Release 0.20.0 (#1257)
1c73e8d 
## Release v0.20.0

The release brings a couple of new breaking changes from the [`fuel-vm
0.35.0`](https://github.com/FuelLabs/fuel-vm/releases/tag/v0.35.0) with
bugfixes. Check the description of the VM release for more details.

The `fuel-core` release mostly improved the internal codebase but also
brought some breaking changes:
- Removed `Trigger::Hybrid` PoA block trigger mode. Only
`Trigger::Instante` and `Trigger::Interval` are available for block
production now. The main mode for testnets and mainnet will be
`Interval`.
- Removed support for `OpaqueReceipt` and the `Receipt` type doesn't
have the `raw_payload` field anymore.
- A `Receipt` type got two new variants: `Mint` and `Burn`. The
corresponding opcodes emit these new events.
- The `AssetId` is derived from `ContractId` and additional nonce. So
the `ContractId` and `AssetId` can't be the same anymore.

## What's Changed
* bump rocksdb to enable compiling with GCC 13 by @segfault-magnet in
#1219
* setting peer reputation params by @leviathanbeak in
#1202
* Take into account the actually used gas by the transactions and fetch
more transaction by @xgreenx in
#1223
* Use production configuration for `fuel-core` during benches by
@xgreenx in #1227
* Speedup and stabilize unit and integration tests by @xgreenx in
#1231
* test: State benchmarks by @bvrooman in
#1226
* Remove hybrid PoA block trigger mode by @Dentosal in
#1232
* test: Benchmark contract state insertions with DB vs. DB transactions
by @bvrooman in #1230
* multiplatform docker builds by @Voxelot in
#1233
* Fix typo in architecture.md by @eltociear in
#1241
* Expose gas cost in chain info by @MitchTurner in
#1244
* Reuse calculated tx id in executor by @MitchTurner in
#1248
* Fix multi-platform images by @Voxelot in
#1251
* Add logging of the long GraphQL queries for future debug by
@MitchTurner in #1250
* Reused `CheckedTransaction` from transaction pool in the executor by
@xgreenx in #1249
* Bump `fuel-vm` to `0.35.0` version by @xgreenx in
#1256

## New Contributors
* @segfault-magnet made their first contribution in
#1219
* @eltociear made their first contribution in
#1241
* @MitchTurner made their first contribution in
#1244

**Full Changelog**:
v0.19.1...v0.20.0
crypto523 added a commit to crypto523/fuel-core that referenced this pull request Oct 7, 2024
@crypto523
Release 0.20.0 (#1257)
94d4486 
## Release v0.20.0

The release brings a couple of new breaking changes from the [`fuel-vm
0.35.0`](https://github.com/FuelLabs/fuel-vm/releases/tag/v0.35.0) with
bugfixes. Check the description of the VM release for more details.

The `fuel-core` release mostly improved the internal codebase but also
brought some breaking changes:
- Removed `Trigger::Hybrid` PoA block trigger mode. Only
`Trigger::Instante` and `Trigger::Interval` are available for block
production now. The main mode for testnets and mainnet will be
`Interval`.
- Removed support for `OpaqueReceipt` and the `Receipt` type doesn't
have the `raw_payload` field anymore.
- A `Receipt` type got two new variants: `Mint` and `Burn`. The
corresponding opcodes emit these new events.
- The `AssetId` is derived from `ContractId` and additional nonce. So
the `ContractId` and `AssetId` can't be the same anymore.

## What's Changed
* bump rocksdb to enable compiling with GCC 13 by @segfault-magnet in
FuelLabs/fuel-core#1219
* setting peer reputation params by @leviathanbeak in
FuelLabs/fuel-core#1202
* Take into account the actually used gas by the transactions and fetch
more transaction by @xgreenx in
FuelLabs/fuel-core#1223
* Use production configuration for `fuel-core` during benches by
@xgreenx in FuelLabs/fuel-core#1227
* Speedup and stabilize unit and integration tests by @xgreenx in
FuelLabs/fuel-core#1231
* test: State benchmarks by @bvrooman in
FuelLabs/fuel-core#1226
* Remove hybrid PoA block trigger mode by @Dentosal in
FuelLabs/fuel-core#1232
* test: Benchmark contract state insertions with DB vs. DB transactions
by @bvrooman in FuelLabs/fuel-core#1230
* multiplatform docker builds by @Voxelot in
FuelLabs/fuel-core#1233
* Fix typo in architecture.md by @eltociear in
FuelLabs/fuel-core#1241
* Expose gas cost in chain info by @MitchTurner in
FuelLabs/fuel-core#1244
* Reuse calculated tx id in executor by @MitchTurner in
FuelLabs/fuel-core#1248
* Fix multi-platform images by @Voxelot in
FuelLabs/fuel-core#1251
* Add logging of the long GraphQL queries for future debug by
@MitchTurner in FuelLabs/fuel-core#1250
* Reused `CheckedTransaction` from transaction pool in the executor by
@xgreenx in FuelLabs/fuel-core#1249
* Bump `fuel-vm` to `0.35.0` version by @xgreenx in
FuelLabs/fuel-core#1256

## New Contributors
* @segfault-magnet made their first contribution in
FuelLabs/fuel-core#1219
* @eltociear made their first contribution in
FuelLabs/fuel-core#1241
* @MitchTurner made their first contribution in
FuelLabs/fuel-core#1244

**Full Changelog**:
FuelLabs/fuel-core@v0.19.1...v0.20.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leviathanbeak leviathanbeak leviathanbeak left review comments

@xgreenx xgreenx xgreenx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MitchTurner @Voxelot @xgreenx @leviathanbeak