fix(graphql): handle nil value for locations in unified_trace

[codella]

What does this PR do?

It then adds a pre-condition check in serialize_error_locations to handle the case when the provided argument locations is nil, and makesserialize_error_locations a class method for testability.

Motivation:

The logic in serialize_error_locations‎ assumes that locations is always present in the Hash returned by ::GraphQL::Error#to_h, but in reality is only present when if the error can be associated to an AST node (ref). This results in occasional NoMethodErrors being raised by applications using this gem.

Change log entry

Yes. Tracing: Fix NoMethodError in graphql integration when error has no locations. (Added by @ivoanjo)

Additional Notes:

Stack Trace:

NoMethodError: undefined method `map' for nil (NoMethodError)

            locations.map do |location|
                     ^^^^
    from datadog/tracing/contrib/graphql/unified_trace.rb:264:in 'serialize_error_locations'
    from datadog/tracing/contrib/graphql/unified_trace.rb:246:in 'block in add_query_error_events'
    from datadog/tracing/contrib/graphql/unified_trace.rb:214:in 'each'
    from datadog/tracing/contrib/graphql/unified_trace.rb:214:in 'add_query_error_events'
    from datadog/tracing/contrib/graphql/unified_trace.rb:70:in 'block in execute_query'
    from datadog/tracing/contrib/graphql/unified_trace.rb:182:in 'block in trace'
    from datadog/tracing/trace_operation.rb:243:in 'block in measure'
    from datadog/tracing/span_operation.rb:167:in 'measure'
    from datadog/tracing/trace_operation.rb:243:in 'measure'
    from datadog/tracing/tracer.rb:425:in 'start_span'
    from datadog/tracing/tracer.rb:166:in 'block in trace'
    from datadog/tracing/context.rb:45:in 'activate!'
    from datadog/tracing/tracer.rb:165:in 'trace'
    from datadog/tracing.rb:29:in 'trace'
    from datadog/tracing/contrib/graphql/unified_trace.rb:161:in 'trace'
    from datadog/tracing/contrib/graphql/unified_trace.rb:50:in 'execute_query'
    from graphql/execution/interpreter.rb:70:in 'block (3 levels) in run_all'
    from graphql/dataloader/null_dataloader.rb:19:in 'append_job'
    from graphql/execution/interpreter.rb:58:in 'block (2 levels) in run_all'
    from graphql/execution/interpreter.rb:54:in 'each'
    from graphql/execution/interpreter.rb:54:in 'each_with_index'
    from graphql/execution/interpreter.rb:54:in 'block in run_all'
    from graphql/tracing/trace.rb:40:in 'execute_multiplex'
    from datadog/tracing/contrib/graphql/unified_trace.rb:44:in 'block in execute_multiplex'
    from datadog/tracing/contrib/graphql/unified_trace.rb:180:in 'block in trace'
    from datadog/tracing/trace_operation.rb:243:in 'block in measure'
    from datadog/tracing/span_operation.rb:167:in 'measure'
    from datadog/tracing/trace_operation.rb:243:in 'measure'
    from datadog/tracing/tracer.rb:425:in 'start_span'
    from datadog/tracing/tracer.rb:166:in 'block in trace'
    from datadog/tracing/context.rb:45:in 'activate!'
    from datadog/tracing/tracer.rb:165:in 'trace'
    from datadog/tracing.rb:29:in 'trace'
    from datadog/tracing/contrib/graphql/unified_trace.rb:161:in 'trace'
    from datadog/tracing/contrib/graphql/unified_trace.rb:44:in 'execute_multiplex'
    from graphql/execution/interpreter.rb:38:in 'run_all'
    from graphql/schema.rb:1492:in 'multiplex'
    from graphql/schema.rb:1468:in 'execute'
    from app/controllers/graphql_controller.rb:19:in 'execute'

How to test the change?

I was expecting bundle exec rake test:graphql to cover the code changes made in contrib/graphql/unified_trace.rb, but it looks like that's not the case. Any suggestions on how to proceed or how to add the necessary code coverage are very welcome!

[ivoanjo]

Hey @codella, thanks for spotting this and sending a fix our way!

If you happen to have the stack trace of one of those NoMethodErrors, it would be cool for us to look into reproducing and see if there's any more situations we might be missing. If not -- that's cool too!

[codella]

Hey @codella, thanks for spotting this and sending a fix our way!

If you happen to have the stack trace of one of those NoMethodErrors, it would be cool for us to look into reproducing and see if there's any more situations we might be missing. If not -- that's cool too!

Hey! Thanks for being so snappy! 😲 I updated the description with the stack trace 🚀

[codella]

@ivoanjo maybe you can help me. I am running the GraphQL tests with bundle exec rake test:graphql, but it doesn't look like they are covering serialize_error_locations 🤷🏽 I even temporarily raised an exception inside serialize_error_locations to see some test fail, but all I got was 100% green tests 😬

[ivoanjo]

Ah, I think you want bundle exec rake test:graphql_unified_trace_patcher. I was able to see a failing test suite when I broke serialize_error_locations with this.

I believe we have them separate because this functionality depends on modern graphql gem versions, yet we still want to support (and test with) the old ones.

[codella]

@ivoanjo I've been spending some time trying to add a test in unified_trace_patcher_spec.rb / test_schema_examples.rb to replicate the situation where GraphQL::ExecutionError doesn't have the ast_node attribute set, but I couldn't manage it.

Do you have any pointers, or alternative ways to write a test for it? I'm a bit puzzled that I couldn't get a test to replicate the issue, given that GraphQL::ExecutionError allows ast_node to be nil and my app can raise GraphQL::ExecutionError without setting ast_node 🤷🏽‍♂️

[ivoanjo]

Good question... I spent a few minutes looking into the graphql gem and our tests and how we may trigger the issue to then test the fix, and I also couldn't trigger the error. TBH I think the fact that this is proving hard to test via that route is suggesting to me that it's not the right way to go, given what we want to test here.

Specifically, we want to test that some specific query in some situation doesn't break. But if this case is possible (you've definitely seen it in production) but awkward to trigger (we've both not had success at this), I'm thinking even if we find a way to trigger the situation on the current gem version, what if in a future version, the error actually is slightly different, and maybe doesn't trigger in the same way, the dd-trace-rb test may start passing for the wrong reasons (doesn't break -- but perhaps not because the code isn't testing for nil).

TL;DR My suggestion here is moving away from integration testing and into a simpler unit test: one that directly tests serialize_error_locations or add_query_error_events which mock data. I think for a "doesn't break on nil" case, that should be both fine and more resistant to future changes in the graphql gem.

[codella]

@ivoanjo alright, I extracted both add_query_error_events and serialize_error_locations in their own module ErrorSpanEvent. This way I could easily test the change. I hope it's not too invasive. If you have any suggestions, let me know!

[ivoanjo]

This way I could easily test the change. I hope it's not too invasive. If you have any suggestions, let me know!

Hmmm, to be honest I'm not the biggest fan of how it looks in this version because the ErrorSpanEvent module appears separate from UnifiedTrace at first glance but depends extremely deeply on it -- e.g. it needs so many things from UnifiedTrace that I'd expect many changes to UnifiedTrace will probably be reflected here. 🤔

What do you think of going back to the previous state, and making serialize_error_locations a self method on UnifiedTrace? That way we could directly test it without having to extract it (I hesitate on extracting only that method, as it seems too small to live alone). 👀

Another useful thing to adjust is unified_trace.rbs (the type declarations) -- if we change them to

def serialize_error_locations: (Array[Hash[String, Integer]]? locations) -> Array[String]

# or in my suggested version

def self.serialize_error_locations: (Array[Hash[String, Integer]]? locations) -> Array[String]

then the typechecker also starts complaining if the code doesn't deal with nil.

WDYT? 😄

[codella]

@ivoanjo super legit suggestion! I implemented it as you said--let me know if I missed anything!

[ivoanjo]

👍 Perfect, thank you!

Merging this may take a bit on our side -- I'll be out for a few days + our current setup needs some manual prodding of CI for community contributions. But I'll make sure this gets included in the upcoming v2.23! :)

[codella]

Thank you @ivoanjo for your help getting this right!