test: e2e testing for OpenTelemetry Tracing #17458
Conversation
|
|
| format: StderrLogFormat::Json, | ||
| filter: Targets::default(), | ||
| }, | ||
| opentelemetry: Some(OpenTelemetryConfig { |
There was a problem hiding this comment.
Are you referring to the opentelemetry field? That cannot be None because otherwise we wouldn't setup the opentelemetry tracing layer which is necessary for the test
|
I hit rebuild in buildkite. Some clippy warnings going on now. |
Thanks! Fix those warnings, now locally running |
|
@ParkMyCar nice find on |
|
Thanks @guswynn :) |
| @@ -339,6 +379,7 @@ pub struct Server { | |||
| pub runtime: Arc<Runtime>, | |||
| _temp_dir: Option<TempDir>, | |||
| pub metrics_registry: MetricsRegistry, | |||
| pub _tracing_guard: Option<TracingGuard>, | |||
There was a problem hiding this comment.
Tiniest little nit, for future reference: the leading underscore is meant to silence the unused field checker! If this is pub it doesn't need the leading underscore, because Rust is smart enough to realize that some downstream crate might use it. (But probably this shouldn't be pub?)
There was a problem hiding this comment.
Great catch! Definitely doesn't need to be pub, let me fix that.
It also feels like this should be a Clippy lint, so I put up rust-lang/rust-clippy#10283, we'll see what the team thinks :)
There was a problem hiding this comment.
Since I know you're interested :) that Clippy lint finally got merged!
Did anyone point you at our guide for this? https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide.md#cockroachdb |
Ah ha! I hadn't seen that yet, thanks for linking it |
Motivation
Fixes MaterializeInc/database-issues#4896
This PR adds an e2e test for OpenTelemetry Tracing, based off of the initial attempt in 2166011.
Specifically it does the following:
with_enable_tracingoption to the testingConfigbuilder inenvironmentdTracingHandle, which is then used byenvironmentdwhen runningtest_emit_tracing_notice, which asserts that after running aSELECTstatement, aNOTICEis emitted, that contains atrace_idNote: The issue with the original attempt in 2166011 is it immediately dropped the
TracingGuard. What's tricky is at first glance it seems like aTracingGuardonly pertains to Sentry, but if you look at theDropimpl you'll see that when Drop-ed theTracingGuardalso shuts down the OpenTelemetry subscriber, which is why in the initial attempt we didn't receive the NOTICE.To prevent similar issues with
TracingGuardin the future, I marked it asmust_use. So if someone ignores it/drops it immediately, it'll throw a warning.Tips for reviewer
To test this I ran it against a local PostgreSQL DB, and as such I had to comment out
ALTER TABLE ... CONFIGURE ZONEinpersistandstash. I don't think this should effect the test, but if it does, I'd be more than happy to run it against CockroachDB, if you could provide some instructions on how to setup a dev environment :)Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way) and therefore is tagged with aT-protolabel.