fix: GraphQL validate events #557
Conversation
swalkinshaw
requested review from
ahayworth,
arielvalentin,
dazuma,
ericmustin,
fbogsany,
mwear,
plantfansam,
robbkidd and
robertlaurin
as code owners
| super.tap do |response| | ||
| errors = response[:errors]&.compact&.map(&:to_h)&.to_json | ||
| unless errors.nil? | ||
| errors = response[:errors]&.compact&.map(&:to_h) || [] |
There was a problem hiding this comment.
This might still be overly defensive but the root cause is that the graphql-ruby gem always returns an array: https://github.com/rmosolgo/graphql-ruby/blob/bde9b64d7df0cdc28045a9231d3219a800052ffc/lib/graphql/static_validation/validator.rb#L54-L56
Fix the logic for adding `graphql.validation.error` events on `validate` spans. The `errors` variable would always end up being a serialized `"[]"` empty array which wasn't nil and an empty `graphql.validation.error` event would be added.
swalkinshaw
force-pushed
the
graphql-instrumentation-fix-validation-events
branch
from
2b648c1 to
acd1455
Compare
| 'graphql.validation.error', | ||
| attributes: { | ||
| 'message' => errors | ||
| 'message' => errors.to_json |
There was a problem hiding this comment.
I assume that GraphQL depends on JSON being required. Is that correct?
There was a problem hiding this comment.
There was a problem hiding this comment.
I hate to do this but I am feeling a little worried about JSON serialization failing for some reason. Is there anything in the errors object that may link back to objects generated by frameworks like Rails?
I have been bitten too many times by as_json called on X without :only option. You must explicitly declare the keys you wish to serialize
There was a problem hiding this comment.
Something else comes to mind. Is there a community consensus around semconv for these kinds of validation errors?
Should we submit an otep for this? cc: @open-telemetry/ruby-contrib-approvers ?
There was a problem hiding this comment.
I hate to do this but I am feeling a little worried about JSON serialization failing for some reason. Is there anything in the errors object that may link back to objects generated by frameworks like Rails?
I don't think that's possible since this is only for static validation errors specifically, which are built by the graphql-ruby gem. Here's the representation: https://github.com/rmosolgo/graphql-ruby/blob/f05ea569af0ceea6eea41919b7bb61f3eb06f2cd/lib/graphql/static_validation/error.rb
This to_json existed before as well so it's not new.
There was a problem hiding this comment.
OK thanks. I have created a discussion here in the hopes to get some eyes/opinions about what to do about this going forward.
There was a problem hiding this comment.
Seems like there is a tracking issue for this now: open-telemetry/semantic-conventions#182
Given that there isn't a consensus and the semconv is evolving I am going to take another look at the PR Thank you again for your contributions!
There was a problem hiding this comment.
This is rather curious:
It records the validation errors using recordException without a backtrace.
instrumentation/graphql/lib/opentelemetry/instrumentation/graphql/tracers/graphql_trace.rb
Outdated
Show resolved
Hide resolved
instrumentation/graphql/lib/opentelemetry/instrumentation/graphql/tracers/graphql_tracer.rb
Outdated
Show resolved
Hide resolved
instrumentation/graphql/test/instrumentation/graphql/tracers/graphql_tracing_test.rb
Outdated
Show resolved
Hide resolved
Co-authored-by: Ariel Valentin <[email protected]>
4 participants
Fix the logic for adding
graphql.validation.errorevents onvalidatespans.The
errorsvariable would always end up being a serialized"[]"value which wasn't nil and an emptygraphql.validation.errorevent would be added.