STAC-25469: map all span attrs - #15
Conversation
| pattern: "db.(.*)" | ||
| target: "db.${1}" | ||
| pattern: (.*)" | ||
| target: "${1}" |
There was a problem hiding this comment.
Won't this pull in way too many attributes? These spans can also contain attributes with the SQL query template as value (this was already a problem with db.* I realize now), so would result in tons of updates to the component (every database call might trigger an update now).
There was a problem hiding this comment.
A valid concern, but not that many from what I've observed. I first added more pattern/target pairs for server.* and net.*, but then thought we could be missing other useful info while still in this "maturing" phase.
An alternative is to have a specific tag map of all the attributes we want (covering legacy and new attributes), but then they need to go under optional. But since db attributes could theoretically change, I'm not sure we want to be that restrictive right now.
There was a problem hiding this comment.
Copying in all attributes can have much worse effects: it triggers updates to the component (and change events) for potentially every span that comes in (which can be every query that is executed, so hundreds or more per second). This will eat up resources (processing and disk)
Look at our own HBase for example (this one I hadn't even expected): it has an attribute with the thread name, which looks like a fork-join pool that has a counter for each new thread it creates.
Thinking more about this, for the long term we probably need to come up with a protection against this. We can't expect everyone to think through all the consequences either.
There was a problem hiding this comment.
Well, not can, has - already observed this while trying out this mapping. Being specific in the tag mappings is necessary it seems.
There was a problem hiding this comment.
Regarding the protection: I think it should be doable to extend the existing tag mapping validation with a gate that prevents all attributes to be mapped (for dev/debug purposes it would be nice to disable such a gate).
| @@ -99,8 +99,8 @@ nodes: | |||
| pattern: "(.*)" | |||
There was a problem hiding this comment.
I see we also, already, include all resource attributes (on the original otel stackpack we only had a few). I'm still in doubt if that's a good idea or not (a restart will trigger an update because PID, etc will change). In the end I think I actually like it.
There was a problem hiding this comment.
Good observation. Tbh, I don't think a lot of deep thought has gone into the tag mappings until now - mostly copy-paste from the first component mapping that was created. With semconv, we should be able to reliably declare fixed tag mapping target/pattern pairs so the mappings/presentations remain stable - we can be selective with the lifecycle attributes like you've pointed out so we don't have "no-op"/heartbeat updates that are unnecessary.
|
Closing this PR in light of a discussion where a more in-depth/thorough approach/fix for databases/messaging mapping needs to be done. |
2 participants
No description provided.