telemetry: Reduce cardinality of some otel span names#3558
Conversation
I tried to follow OpenTelemetry naming guidance, e.g. https://opentelemetry.io/docs/specs/semconv/db/database-spans/#name
lann
force-pushed
the
otel-reduce-span-name-cardinality
branch
from
d8b23cd to
6165f42
Compare
| otel.name = "publish", | ||
| messaging.operation = "publish", | ||
| messaging.system = "mqtt", | ||
| messaging.destination.name = topic, |
There was a problem hiding this comment.
Moved topic here (per otel semantic conventions)
rylev
left a comment
rylev
left a comment
There was a problem hiding this comment.
I'm really not sure the query parsing is worth the hassle. I agree with the goal of this PR, but I'm not convinced that we need to be this clever.
|
|
||
| impl<C: Client> v2::HostConnection for InstanceState<C> { | ||
| #[instrument(name = "spin_outbound_mysql.open", skip(self, address), err(level = Level::INFO), fields(otel.kind = "client", db.system = "mysql", db.address = Empty, server.port = Empty, db.namespace = Empty))] | ||
| #[instrument(name = "spin_outbound_mysql.open", skip(self, address), err(level = Level::INFO), |
There was a problem hiding this comment.
Nit: shouldn't we use skip_all instead of skip(self, address)?
There was a problem hiding this comment.
That sounds fine but I don't really want to audit all arguments in this PR.
calebschoepp
left a comment
calebschoepp
left a comment
There was a problem hiding this comment.
Overall I think this PR is plugging a big gap in our tracing cardinality risk — I could be convinced to approve this ~as is.
However, I'm feeling vaguely uncomfortable about two things.
- The implementation of
sql_span_nameseems like an endlessly deep tunnel of edge cases. It seems like the kind of code we're going to have to keep coming back to time after time to fix little things. I like that it is dependency free, but did we fully rule out that there isn't an existing library that could solve this problem for us? - It feels like we have a mix of OTel convention span names and "spin style" span names. Maybe this is just fine and we can't do much better, but I feel weird about them being mixed. (I'm also aware this is a bit outside the scope of your PR so feel free to ignore this concern)
| async fn execute( | ||
| &mut self, | ||
| connection: Resource<v2::Connection>, | ||
| statement: String, |
There was a problem hiding this comment.
Do we need to do anything to obfuscate the statement in the attribute?
There was a problem hiding this comment.
That one is tricky. There really shouldn't be anything sensitive in the statement, but of course that isn't going to stop people from doing it. My current opinion is that we should leave it up to the user's telemetry pipeline to deal with that.
|
I'm fine dropping the SQL stuff but if we aren't going to try to converge on otel conventions then we should come up with our own rationale for what we do want to converge on. |
3 participants
This PR removes a lot of detail from some span names.
https://github.com/open-telemetry/opentelemetry-specification/blob/v1.56.0/specification/trace/api.md#span
Given that a large number of
#[instrument(...)]attrs were being touched anyway, I took this opportunity to split most of them across two (or more) lines, including some lines (but not files) that weren't otherwise updated.