improvement: Igni/test span bump

[o0Ignition0o]

🔧 improvement: Test_span: add more powerful filtering capabilities, to remove non determinism in tests.

This PR bumps test-span to version 0.2.0 which allows us to add log level filters according to our dependencies; eg:

#[test_span(tokio::test)]
#[target(reqwest=tracing::Level::INFO)]
#[target(reqwest_tracing=tracing::Level::INFO)]
#[target(hyper::client=tracing::Level::INFO)]

etc.

This allows us to remove some deps logs that are non deterministic at the moment.

[o0Ignition0o]

ogod forgot to rebase 😭

[garypen]

I'm not sure if my comments are correct, but worth asking I guess.

[garypen]

Don't we actually want:

#[level(tracing::Level::ERROR)]
#[target(apollo_router=tracing::Level::DEBUG)]
#[target(apollo_router_core=tracing::Level::DEBUG)]

to reduce the prospect of interference?

The above should mean we only get messages at level ERROR from things which aren't apollo_router or apollo_router_core. I believe that's the goal, but I could be on the wrong track...

[o0Ignition0o]

Hmm that's a very interesting comment!

the default level for all logs in test_span is debug, unless specified otherwise, which means this tests kind of unfolds to:

#[level(tracing::Level::DEBUG)]
#[target(reqwest=tracing::Level::INFO)]
#[target(reqwest_tracing=tracing::Level::INFO)]
#[target(hyper::client=tracing::Level::INFO)]

which effectively means we're tracking spans and logs at the DEBUG level unless it's from reqwest, reqwest_tracing, and hyper.

What teases my mind is that your comment may call for a workspace level and a dependencies level, which I'll happily open an issue for in test_span, if it sounds good to you?

[garypen]

The generally accepted default for log/tracing levels is INFO. That means it's a bit "surprising" (as in principle of least surprise) that test_span's default level is DEBUG.

That aside, I think that if we were going to vary from INFO as a default, then we should probably vary to ERROR, to make logging an (almost) "opt-in" decision and thus attempt to minimize random variability when new dependencies are added.

I'd probably prefer: default is INFO (so no surprises) and also recommend that people configure a default of ERROR (as I did) and opt-in for the stuff they want to capture rather than specifying what they want to exclude (which is more fragile and likely to break as crates are added/removed).

My second preference would be to set the default to ERROR and again require an opt-in for stuff you are interested in.

[o0Ignition0o]

I feel like defaulting to INFO would make a lot of sense indeed, I'm not quite sure ERROR would be a sensible default for a span / logs snapshots crate.

However when it comes to apollo_rs setting the log level to ERROR (or INFO) wouldn't show interesting spans, thus defeating the purpose of "having regression tests that prevents us from breaking the tracing spans layout we have in place when it comes to processing a graphql request".

I'm opening an issue!

[garypen]

But wouldn't you just add an entry for apollo_rs? e.g.:

#[target(apollo_rs=tracing::Level::DEBUG)]

[o0Ignition0o]

that works as well!

router/apollo-router/tests/integration_tests.rs

Line 69 in d088afd

#[target(apollo_router=tracing::Level::DEBUG)]

[garypen]

This looks great!

[cecton]

We write versions in full so we have a track of what was the last working version

Suggested change

	test-span = "0.3"
	test-span = "0.3.0"

[o0Ignition0o]

ok, will open a followup then!

[abernix]

Just creating a link to the upstream PR: apollographql/test-span#11