[BUG?]: graphql resolver depth filter depends on path collapsing

[ab-pm]

Bug Report

I notice while inspecting the code of the graphql instrumentation/plugin that the depth config option has a rather peculiar behaviour. The shouldInstrument function

dd-trace-js/packages/datadog-plugin-graphql/src/resolve.js

Lines 110 to 119 in 5fd961c

	function shouldInstrument (config, path) {
	let depth = 0
	for (const item of path) {
	if (typeof item === 'string') {
	depth += 1
	}
	}
	return config.depth < 0 || config.depth >= depth
	}

counts the number of non-numeric path segments, i.e. treating a selection on a field returning a list (or list of lists, etc) as a single level. However, with the collapse config option enabled (which is the default), the numbers in the path get transformed into the string '*'

dd-trace-js/packages/datadog-plugin-graphql/src/resolve.js

Line 141 in 5fd961c

.map(segment => typeof segment === 'number' ? '*' : segment)

before the path array is passed into shouldInstrument:

dd-trace-js/packages/datadog-plugin-graphql/src/resolve.js

Lines 81 to 83 in 5fd961c

	const path = getPath(info, this.config)
	if (!shouldInstrument(this.config, path)) return

So when collapsing is turned off, less path segments are counted and more (deeper) resolver calls are instrumented. You can easily test this by

Reproduction Code

You can try this yourself by running the "graphql with a depth >=1 should only instrument up to the specified depth" test. Change

dd-trace-js/packages/datadog-plugin-graphql/test/index.spec.js

Line 1528 in 5fd961c

return agent.load('graphql', { depth: 2 })

to

          return agent.load('graphql', { depth: 2, collapse: false })

and the test fails because the trace now also contains spans for friends.0.name and friends.1.name.

Question

Is this considered a bug or a feature? Or an oversight? What is the desired behaviour?

I'm about to open a PR with some optimisations in this place, and would like to know if it will need to keep exact compatibility with the current behaviour.

[BridgeAR]

Thank you for the report. We generally need to have a close look at the GraphQL instrumentation. I believe we have a couple of small issues in that need addressing. I will try to get some attention on this soon (meaning a couple of weeks).

If you want to escalate this, please reach out to our customer support. That way this could be looked at fastest.

[ab-pm]

Thanks for your reply! This bug is a non-issue for us, but there are some features we are missing (and willing to implement in a PR, once there is enough understanding). We'll reach out to customer support to help with prioritisation.

We generally need to have a close look at the GraphQL instrumentation. I believe we have a couple of small issues in that need addressing

Do you have a list of these? I'm curious what else doesn't behave as it should, so that I could address it when passing across.