Telemetry fixes

[calebschoepp]

Summary

• Fixes bug that slipped in with Upgrade OTel #2834.
• Improves a comment.
• Removes potentially sensitive values from OTel traces.

Questions

• Do we want to remove DB addresses from OTel traces b/c they might contain credentials inline?
• Do we want to remove key values everywhere from OTel traces? This seems valuable enough to me that I think we should just keep it and document it.

Fixe #2816

[rylev]

Seems like an improvement. As for your questions:

• re: DB addresses - this seems like a smart idea to remove
• key-value: I think we definitely want to remove values - I can see the argument both for keeping and removing keys. Perhaps we keep them for now, and see what people think over time?

[lann]

Do we want to remove key values everywhere from OTel traces? This seems valuable enough to me that I think we should just keep it and document it.

We could perhaps drop it by default but allow opting-in by level, e.g.

#[instrument(..., fields(key = Empty))]
...
// I believe this would allow e.g. RUST_LOG=spin_factor_key_value=debug
if tracing::enabled!(Level::DEBUG) {
  span.record("key", key);
}

[calebschoepp]

Ready for another round of review. I think that we should just keep keys for now and we can handle feedback in the future. I feel that it is a reasonable requirement that PII should be kept out of keys.

[calebschoepp]

Do we want to remove key values everywhere from OTel traces? This seems valuable enough to me that I think we should just keep it and document it.

We could perhaps drop it by default but allow opting-in by level, e.g.

#[instrument(..., fields(key = Empty))]
...
// I believe this would allow e.g. RUST_LOG=spin_factor_key_value=debug
if tracing::enabled!(Level::DEBUG) {
  span.record("key", key);
}

I tried this and it didn't work.

[rylev]

I still have questions, but nothing that should block if you feel this is ready to merge.

[calebschoepp]

Ready for one more review and then for someone to merge it if they think it's good to go.

[rylev]

@calebschoepp looks like the failure in CI is legit:

error: expected statement after outer attribute
 --> crates/factor-outbound-networking/src/lib.rs:258:1
  |
3 | #[instrument(fields(db.address = Empty, server.port = Empty, db.namespace = Empty))]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[calebschoepp]

@calebschoepp looks like the failure in CI is legit:

error: expected statement after outer attribute
 --> crates/factor-outbound-networking/src/lib.rs:258:1
  |
3 | #[instrument(fields(db.address = Empty, server.port = Empty, db.namespace = Empty))]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Fixed