[network metrics] instance data link schema #6402
Conversation
sled-agent/src/instance.rs
Outdated
| // We expect to match NIC slots to OPTE port slots. | ||
| .find(|nic| nic.slot == port.slot()) |
There was a problem hiding this comment.
There should always be one port for each guest NIC. Should we do something here if that's not the case?
There was a problem hiding this comment.
@bnaecker after thinking about it, and knowing that a requested nic should match to a port, I've updated to throw an error otherwise. Thoughts?
This is related to the ongoing work in [plumbing instance/guest metrics](oxidecomputer/propolis#742) through to an oximeter producer in propolis. Includes: - Rewriting the comment for `trait Target` in Oximeter to not being prepped for deprecation, as it does come in handy for custom needs - The main differentiating field among link information here is `interface_id`, which is exposed to propolis via the client's `NetworkInterfaceRequest`. We currently match the `nic.slot` to the `port.slot` to pass which of the requested nics `interface_id` uuids are part of the request.
zeeshanlakhani
force-pushed
the
zl/instance-data-link
branch
from
74e7b8e to
b6dbacc
Compare
zeeshanlakhani
force-pushed
the
zl/instance-data-link
branch
from
b6dbacc to
f5edc3c
Compare
| [fields.interface_id] | ||
| type = "uuid" | ||
| description = "The ID of the data link" |
There was a problem hiding this comment.
Sorry for all the back and forth on this one, I appreciate your patience.
Seeing this called interface_id with a description referring to a "data link" makes me wonder if we've gotten the target name wrong. We picked instance_data_link to match up with the sled_data_link and switch_data_link targets. That's nice, except the public API doesn't refer to "data links" anywhere. As this field name indicates, the customer operates on NetworkInterface objects through the API. I worry it'd be a bit confusing to say, "Hey, I wonder what this interface is doing? Let me go look for a timeseries about 'interfaces'", only to find out that the timeseries is really instance_data_link:bytes_sent or whatever. I think I'd find that annoying or confusing, learning it for the first time.
What do you think about renaming the target to instance_network_interface or virtual_network_interface, or possibly just network_interface?
There was a problem hiding this comment.
Yeah, keeping to the naming around instance-data-link has been confusing. I'd say just network_interface and therefore network-interface.toml, right?
Then this description is "The ID of the network interface". Thoughts @bnaecker?
There was a problem hiding this comment.
Sweet, yeah the description sounds right in that case. I would personally vote for instance_network_interface, just to avoid any possibility of confusion. That sound OK to you?
There was a problem hiding this comment.
Oh, and I would s/data link/network interface in the other descriptions too.
There was a problem hiding this comment.
Sure, instance_network_interface it is. I'll push up a new PR.
|
Closing in favor of another PR capturing the correct naming. |
This is related to the ongoing work in plumbing instance/guest metrics through to an oximeter producer in propolis.
Includes:
trait Targetin Oximeter to not being prepped for deprecation, as it does come in handy for custom needsinterface_id, which is exposed to propolis via the client'sNetworkInterfaceRequest. We currently match thenic.slotto theport.slotto pass which of the requested nicsinterface_iduuids are part of the request.Dependencies