-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Replace metricsmap-bpf-prom-sync with Prometheus Collector pattern #27370
Conversation
Commit 971a40e9b4830fd094a9fb641dd97b0832be9a8e does not contain "Signed-off-by". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
Commit 971a40e9b4830fd094a9fb641dd97b0832be9a8e does not contain "Signed-off-by". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
Hi @carnerito, thank you for this contribution. Could you sign off on your commit? https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
@dylandreimerink Can I squash all commits and force-push? Update |
7de9995
to
8fb7280
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carnerito Thanks for the updates! Some minor stylistic changes are required, with strong encouragement to fix some of the inherited language touched by this PR.
42c4cde
to
d1988ae
Compare
1a47832
to
42e3b7b
Compare
Previously, the 'metricsmap-bpf-prom-sync' controller was responsible for periodically collecting cilium_datapath drop and forward metrics. This commit introduces the metricsmapCollector, which implements the Prometheus Collector interface within the metricsmap package. As a result, the aforementioned controller has been removed. The metricsmapCollector is registered within the global Prometheus registry during the initialization of metricsmap. The metrics.Register function has been modified to propagate errors from the registry.Register function instead of simply overriding it with nil. The metricsmapCollector comprises two metrics maps: forwardedMetricsMap and droppedMetricsMap. These maps are populated within a callback function passed to the IterateWithCallback function. This approach serves two primary purposes: 1. Separation of Map Iteration and Metric Update: By separating the iteration over the BPF map and the updating of Prometheus metrics, the implementation ensures that no partial metrics are exposed in case of map iteration failure. 2. Normalization of exposed Metrics: Unlike the statement in the bpf/lib/metrics.h comments, which suggests exposing only one reason label for forwarded metrics, the eBPF code exposes multiple reasons. Through testing, it was found that reasons 0 and 3 were exposed which triggered this error in prometheus client: prometheus/client_golang#242 Furthermore, cilium command has indirect dependency on metricsmap package. To prevent metrics map initialization to happen each time cilium command is executed, metricsmap is converted to hive Cell and injected in cilium daemon Infrastructure module. For details see this comment: cilium#27370 (comment). Fixes: cilium#27058 Signed-off-by: Boris Petrovic <carnerito.b@gmail.com>
I've rebased the PR to re-run the tests. |
/test |
Previously, the 'metricsmap-bpf-prom-sync' controller was responsible for periodically collecting cilium_datapath drop and forward metrics. This commit introduces the metricsmapCollector, which implements the Prometheus Collector interface within the metricsmap package. As a result, the aforementioned controller has been removed. The metricsmapCollector is registered within the global Prometheus registry during the initialization of metricsmap. The metrics.Register function has been modified to propagate errors from the registry.Register function instead of simply overriding it with nil. The metricsmapCollector comprises two metrics maps: forwardedMetricsMap and droppedMetricsMap. These maps are populated within a callback function passed to the IterateWithCallback function. This approach serves two primary purposes: 1. Separation of Map Iteration and Metric Update: By separating the iteration over the BPF map and the updating of Prometheus metrics, the implementation ensures that no partial metrics are exposed in case of map iteration failure. 2. Normalization of exposed Metrics: Unlike the statement in the bpf/lib/metrics.h comments, which suggests exposing only one reason label for forwarded metrics, the eBPF code exposes multiple reasons. Through testing, it was found that reasons 0 and 3 were exposed which triggered this error in prometheus client: prometheus/client_golang#242 Furthermore, cilium command has indirect dependency on metricsmap package. To prevent metrics map initialization to happen each time cilium command is executed, metricsmap is converted to hive Cell and injected in cilium daemon Infrastructure module. For details see this comment: cilium#27370 (comment). Fixes: cilium#27058 Signed-off-by: Boris Petrovic <carnerito.b@gmail.com>
42e3b7b
to
ec983f9
Compare
/test |
@aanm Cilium build was failing because of unused import. It's fixed now, could you please rerun tests? |
@carnerito Thanks, can you add the last commit as part of 8e741df? We try that each commit is in a "buildable state" |
@aanm Just to be sure, do you want me to squash all commits and force-push? |
@carnerito I didn't look at all commits but now that you mentioned it, yes please squash all commits together and force-push. Thank you! |
Previously, the 'metricsmap-bpf-prom-sync' controller was responsible for periodically collecting cilium_datapath drop and forward metrics. This commit introduces the metricsmapCollector, which implements the Prometheus Collector interface within the metricsmap package. As a result, the aforementioned controller has been removed. The metricsmapCollector is registered within the global Prometheus registry during the initialization of metricsmap. The metrics.Register function has been modified to propagate errors from the registry.Register function instead of simply overriding it with nil. The metricsmapCollector comprises two metrics maps: forwardedMetricsMap and droppedMetricsMap. These maps are populated within a callback function passed to the IterateWithCallback function. This approach serves two primary purposes: 1. Separation of Map Iteration and Metric Update: By separating the iteration over the BPF map and the updating of Prometheus metrics, the implementation ensures that no partial metrics are exposed in case of map iteration failure. 2. Normalization of exposed Metrics: Unlike the statement in the bpf/lib/metrics.h comments, which suggests exposing only one reason label for forwarded metrics, the eBPF code exposes multiple reasons. Through testing, it was found that reasons 0 and 3 were exposed which triggered this error in prometheus client: prometheus/client_golang#242 Furthermore, cilium command has indirect dependency on metricsmap package. To prevent metrics map initialization to happen each time cilium command is executed, metricsmap is converted to hive Cell and injected in cilium daemon Infrastructure module. For details see this comment: cilium#27370 (comment). Fixes: cilium#27058 Signed-off-by: Boris Petrovic <carnerito.b@gmail.com>
b5f07e9
to
6711c88
Compare
@aanm Done |
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carnerito Thank you for this PR and updates Boris!!
Required tests are passing. Seems to be mainly waiting on @cilium/sig-foundations review due to the cells.go change. |
Removing my review, I was only involved here as triager trying to keep PRs moving. |
Is it me or this pull request involuntary renamed a bunch of metrics? For example, what used to be called |
Ah, perfect! Thanks! The fix simply didn't reach my PR yet 🙂 |
Previously, the 'metricsmap-bpf-prom-sync' controller was responsible for periodically collecting cilium_datapath drop
and forward metrics.
This PR introduces the metricsmapCollector, which implements the Prometheus Collector interface within the
metricsmap package. As a result, the aforementioned controller has been removed.
The metricsmapCollector is registered within the global Prometheus registry during the initialization of metricsmap.
The metrics.Register function has been modified to propagate errors from the registry.Register function instead of
simply overriding it with nil.
The metricsmapCollector comprises two metrics maps: forwardedMetricsMap and droppedMetricsMap.
These maps are populated within a callback function passed to the IterateWithCallback function.
This approach serves two primary purposes:
Prometheus metrics, the implementation ensures that no partial metrics are exposed in case of map iteration failure.
only one reason label for forwarded metrics, the eBPF code exposes multiple reasons. Through testing, it was found
that reasons 0 and 3 were exposed which triggered this error in prometheus client:
It occurs "collected metric %s %s was collected before with the same name and label values" error in the lastest version prometheus/client_golang#242
Furthermore, cilium command has an indirect dependency on metricsmap package. To prevent metrics map initialization
from happening each time cilium command is executed, metricsmap is converted to hive Cell and injected in cilium daemon
Infrastructure module. For details see this comment: #27370 (comment).
Fixes: #27058
Signed-off-by: Boris Petrovic carnerito.b@gmail.com