Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

how to support struct type value #253

Open
cdkunsong opened this issue Aug 22, 2023 · 10 comments
Open

how to support struct type value #253

cdkunsong opened this issue Aug 22, 2023 · 10 comments

Comments

@cdkunsong
Copy link

cdkunsong commented Aug 22, 2023

hi,
I also want to discuss a issue, whether it is possible to use struct type for value,
although I see in tcp-window-clamps.bpf.c

struct socktest {
     u32 value1;
     u32 value2;
     u32 value3;
}
struct {
    __uint(type, BPF_MAP_TYPE_LRU_HASH);
    __uint(max_entries, 1024);
    __type(key, u64);
    __type(value, struct socktest *);
} tcp_rmem_schedule_enters SEC(".maps");

but it don't use by metrics .
Currently, it seems that a struct type cannot be supported, and it can parse multiple member values.

Can you explain this problem, thanks!

@bobrik
Copy link
Contributor

bobrik commented Aug 23, 2023

Please use markdown when you quote code, it's hard to read code without any indentation.

Prometheus metrics have basic types: counter, gauge, histograms (which is just a bunch of counters). All of these have just float64 as values, as described in prometheus data model:

Given that, I'm not sure what you would do with a struct in a map value.

@ntnx-aleksa
Copy link
Contributor

Hello,

could you consider allowing struct values and similarly having labels and decoders for each struct member? I think this is very useful. I am counting a large amount of values in a struct because they all belong to the same key. For ebpf_exporter I would have to split this struct into a dozen maps with value u64. This will probably have a lot of lookup overhead.
If there is a better alternative or I'm misunderstanding something, please let me know.

Thank you.

@bobrik
Copy link
Contributor

bobrik commented Aug 25, 2023

What is your concrete use-case? What kind of metrics and labels do you want to produce?

@ntnx-aleksa
Copy link
Contributor

Let's say I'm collecting a dozen counters, that is u64 monotonically increasing variables, all related to the same block device - it makes perfect sense to group these together. I have a per-cpu hash map with a u32 key, decoded as majorminor and a struct disk_counters value type.

For example:

struct disk_counters {
    unsigned long counter;
    unsigned long another_counter;
    unsigned long yet_another_counter;
    // ...
}

Currently, I would have to split this struct by creating a new map for each field. I tried just that and profiling the two BPF programs with bpftool shows degraded performance on the ebpf_exporter-compatible program.
I suggest allowing the user to supply a struct and treating the data obtained from libbpfgo.BPFMap.GetValue as a u64 array. Then only one lookup would be performed, and each array member would be a new metric. The names of each metric could be described in the YAML configuration file, similary to how the static_map decoder works.

Another thing is that when I first tried writing a configuration with a struct as the value type, ebpf_exporter gave no warning or error at all. At least the silent failure should be avoided with a warning.

@bobrik
Copy link
Contributor

bobrik commented Aug 31, 2023

Conceptually it makes sense. Can you quantify you performance degradation? How expensive is it with a single map vs multiple maps? Is it visible in overall CPU usage?

@bobrik
Copy link
Contributor

bobrik commented Aug 31, 2023

It's unclear to me how to make complex values atomic. It's trivial to increment a single u64. How would you increment an array of them joined together?

@bobrik
Copy link
Contributor

bobrik commented Aug 31, 2023

I opened #257 to validate value sizes for maps so that there are no silent failures.

@ntnx-aleksa
Copy link
Contributor

It's unclear to me how to make complex values atomic. It's trivial to increment a single u64. How would you increment an array of them joined together?

This is a good point. So far, I've used per-CPU maps to solve this, so that could be a requirement.

@bobrik
Copy link
Contributor

bobrik commented Sep 1, 2023

Could you share the benchmark you're using? I'm still very interesting in these questions: #253 (comment).

@bobrik
Copy link
Contributor

bobrik commented Sep 3, 2023

I made #260, but I can't say I'm a big fan of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants