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

Expose the operation name from juniper_rocket::GraphQLRequest #353

Merged
merged 6 commits into from
May 15, 2019
Merged

Expose the operation name from juniper_rocket::GraphQLRequest #353

merged 6 commits into from
May 15, 2019

Conversation

davidpdrsn
Copy link
Contributor

Measuring the runtime of queries will only tell if there are slow
queries. To find out which queries are slow you need the operation name.

Getting the operation name was previously not possible from a Rocket
request handler. This fixes that.

I ran into this issue recently when implementing performance performance
monitoring for an app using juniper-rocket.

Measuring the runtime of queries will only tell if there are slow
queries. To find out which queries are slow you need the operation name.

Getting the operation name was previously not possible from a Rocket
request handler. This fixes that.
@codecov-io
Copy link

codecov-io commented May 11, 2019

Codecov Report

Merging #353 into master will decrease coverage by 1.95%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #353      +/-   ##
==========================================
- Coverage   88.32%   86.37%   -1.96%     
==========================================
  Files         103      105       +2     
  Lines       14922    15373     +451     
==========================================
+ Hits        13180    13278      +98     
- Misses       1742     2095     +353
Impacted Files Coverage Δ
juniper/src/http/mod.rs 0% <ø> (ø) ⬆️
juniper_rocket/src/lib.rs 90% <93.75%> (+0.23%) ⬆️
juniper/src/macros/object.rs 42.42% <0%> (-52.58%) ⬇️
juniper_codegen/src/util.rs 30.57% <0%> (-38.27%) ⬇️
juniper/src/http/playground.rs 0% <0%> (-25%) ⬇️
juniper/src/parser/tests/value.rs 91.04% <0%> (-5.95%) ⬇️
juniper/src/parser/tests/document.rs 93.1% <0%> (-2.3%) ⬇️
juniper/src/macros/interface.rs 88.63% <0%> (-2.28%) ⬇️
juniper/src/integrations/serde.rs 57.89% <0%> (-0.76%) ⬇️
juniper/src/types/base.rs 91.01% <0%> (-0.7%) ⬇️
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 794568e...84c410d. Read the comment docs.

@theduke
Copy link
Member

theduke commented May 11, 2019

Looks good.

Two nits:

  • Formatting is failing (please run cargo fmt on stable)
  • also update the juniper changelog

theduke
theduke previously approved these changes May 11, 2019
@davidpdrsn
Copy link
Contributor Author

@theduke Done

Copy link
Member

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

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

Looks great! Please add an integration test for juniper and juniper_rocket to make sure we don't regress in the future.

if reqs.len() == 1 {
reqs.get(0).and_then(|req| req.operation_name())
} else {
None
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't we want a list of operations for batch requests for the same usecase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that is true. Fixed in 168a763

@LegNeato LegNeato mentioned this pull request May 12, 2019
@davidpdrsn
Copy link
Contributor Author

cargo test fails for me on nightly, which is necessary for rocket to work. Not quite sure what is wrong. I get this output

$ cargo test
warning: /Users/davidpdrsn/dev/major/juniper-fork/integration_tests/juniper_tests/Cargo.toml: file found to be present in multiple build targets: /Users/davidpdrsn/dev/major/juniper-fork/integration_tests/juniper_tests/src/lib.rs
   Compiling juniper_hyper v0.2.0 (/Users/davidpdrsn/dev/major/juniper-fork/juniper_hyper)
error: internal compiler error: src/librustc_mir/borrow_check/nll/universal_regions.rs:741: cannot convert `ReScope(Node(257))` to a region vid

thread 'rustc' panicked at 'Box<Any>', src/librustc_errors/lib.rs:637:9
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
error: aborting due to previous error


note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.36.0-nightly (3f5152e20 2019-05-08) running on x86_64-apple-darwin

note: compiler flags: -C debuginfo=2 -C incremental --crate-type bin

note: some of the compiler flags provided by cargo are hidden

error: Could not compile `juniper_hyper`.
warning: build failed, waiting for other jobs to finish...
error: internal compiler error: src/librustc_mir/borrow_check/nll/universal_regions.rs:741: cannot convert `ReScope(Node(257))` to a region vid

thread 'rustc' panicked at 'Box<Any>', src/librustc_errors/lib.rs:637:9
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
error: aborting due to previous error


note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.36.0-nightly (3f5152e20 2019-05-08) running on x86_64-apple-darwin

note: compiler flags: -C debuginfo=2 -C incremental

note: some of the compiler flags provided by cargo are hidden

error: Could not compile `juniper_hyper`.

To learn more, run the command again with --verbose.

Exact toolchain is rustc 1.36.0-nightly (3f5152e20 2019-05-08)

@LegNeato
Copy link
Member

The bug you are hitting is rust-lang/rust#59344.

@davidpdrsn
Copy link
Contributor Author

It seems a solution was recently merged. I'll give it another shot on the latest nightly.

@davidpdrsn
Copy link
Contributor Author

I have added an integration test now. I wasn't quite sure if it also made sense to test the path where there is no operation name provided. I decided to trust the typesystem 😊 Let me know if you would like that.

@LegNeato
Copy link
Member

Thanks!

@LegNeato LegNeato merged commit 2518eff into graphql-rust:master May 15, 2019
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