Option to easily discard bytes explicitely without label

[aibor]

As described in the documentation, padding bytes in the counter key struct need to be considered when decoding the values.
The documentation suggests just to include the padding space in the previous value.

In case the structure is not as simple, it can get tricky and I need to define "reserved" fields. Those end up as label unnecessarily.

So, in order to avoid that, with a "discard" decoder I could explicitely discard such padding bytes.

I'd be happy to provide a patch, if you like the idea. Regarding the naming of such a decoder I'm open to suggestions.

Edit: It seems better to just add a simple "discard" or "skip" config flag to the label.

[bobrik]

Do you have an example struct that requires this? If we want to have this functionality, it needs an example attached.

[aibor]

Right now I have a struct like this:

struct inet_frags_counter_key_t {
  u32 ifindex;
  u8 ip_version;
  u8 ip_protocol;
};

Due to alignment, it is 8 byte long. In the config, I can't use size: 3 for the label ip_protocol, because it is an invalid length. So I need to use a config like this:

metrics:
  counters:
    - name: frags_total
      help: number of fragments seen
      labels: 
        - name: interface
          size: 4
          decoders:
            - name: ifname
        - name: ip_version
          size: 1
          decoders:
            - name: uint
        - name: ip_protocol
          size: 1
          decoders:
            - name: uint
        - name: padding_bytes
          size: 2
          decoders:
            - name: uint

The label "padding_bytes" is added just to get the proper map key size.

In order to avoid the unnecessary label, an additional config key (e.g. discard: true) might be a straight forward solution?
So, the config would look like this:

metrics:
  counters:
    - name: frags_total
      help: number of fragments seen
      labels:
        - name: interface
          size: 4
          decoders:
            - name: ifname
        - name: ip_version
          size: 1
          decoders:
            - name: uint
        - name: ip_protocol
          size: 1
          decoders:
            - name: uint
        - size: 2
          discard: true

[bobrik]

I see. Normally I just upsize the struct members (u8 -> u16), which doesn't change the struct size.

#include <stdint.h>

struct inet_frags_counter_key_u8_t {
  uint32_t ifindex;
  uint8_t ip_version;
  uint8_t ip_protocol;
};

struct inet_frags_counter_key_u16_t {
  uint32_t ifindex;
  uint16_t ip_version;
  uint16_t ip_protocol;
};

struct inet_frags_counter_key_u8_t	key_u8	= {};
struct inet_frags_counter_key_u16_t	key_u16 = {};

$ gcc -c -g hole.c

$ pahole --structs hole.o

struct inet_frags_counter_key_u8_t {
	uint32_t                   ifindex;              /*     0     4 */
	uint8_t                    ip_version;           /*     4     1 */
	uint8_t                    ip_protocol;          /*     5     1 */

	/* size: 8, cachelines: 1, members: 3 */
	/* padding: 2 */
	/* last cacheline: 8 bytes */
};
struct inet_frags_counter_key_u16_t {
	uint32_t                   ifindex;              /*     0     4 */
	uint16_t                   ip_version;           /*     4     2 */
	uint16_t                   ip_protocol;          /*     6     2 */

	/* size: 8, cachelines: 1, members: 3 */
	/* last cacheline: 8 bytes */
};

I can see an argument to add discard: true at the end or padding: 2 to ip_protocol, but I'm also a bit worried about enabling two mechanisms to do the same thing. What do you think?

[aibor]

Using longer types definitely is a simple workaround and would works for my use cases as well.

But if it were implemented, I'd prefer your padding key solution. This makes configuration even easier and should work for all relevant cases I can think of (padding at the end or in between, multiple fields with padding).

[bobrik]

I'm open to a PR to add padding support.