[usm] system-probe, add Postgres message counters to the eBPF module #31104
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yuri-lipnesh
added
changelog/no-changelog
component/system-probe
team/usm
Test changes on VMUse this command from test-infra-definitions to manually test this PR changes on a VM: inv aws.create-vm --pipeline-id=50607237 --os-family=ubuntuNote: This applies to commit 3e8d162 |
eBPF complexity changesSummary result: ❔ - needs attention
usm detailsusm [programs with changes]
usm [programs without changes]
This report was generated based on the complexity data for the current branch yuri_USMON-1124_ebpf_postgres_msg_counts (pipeline 50607237, commit 3e8d162) and the base branch main (commit c5069b5). Objects without changes are not reported. Contact #ebpf-platform if you have any questions/feedback. Table complexity legend: 🔵 - new; ⚪ - unchanged; 🟢 - reduced; 🔴 - increased |
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: c5069b5 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | tcp_syslog_to_blackhole | ingress throughput | +0.88 | [+0.83, +0.94] | 1 | Logs |
| ➖ | file_to_blackhole_500ms_latency | egress throughput | +0.11 | [-0.66, +0.88] | 1 | Logs |
| ➖ | quality_gate_idle | memory utilization | +0.10 | [+0.05, +0.14] | 1 | Logs bounds checks dashboard |
| ➖ | uds_dogstatsd_to_api_cpu | % cpu utilization | +0.08 | [-0.63, +0.80] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency | egress throughput | +0.03 | [-0.83, +0.89] | 1 | Logs |
| ➖ | file_to_blackhole_100ms_latency | egress throughput | +0.02 | [-0.68, +0.73] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency_linear_load | egress throughput | +0.02 | [-0.44, +0.48] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api | ingress throughput | +0.01 | [-0.12, +0.14] | 1 | Logs |
| ➖ | file_to_blackhole_300ms_latency | egress throughput | +0.01 | [-0.62, +0.64] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency_http2 | egress throughput | +0.01 | [-0.86, +0.87] | 1 | Logs |
| ➖ | tcp_dd_logs_filter_exclude | ingress throughput | +0.00 | [-0.01, +0.01] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency_http1 | egress throughput | -0.02 | [-0.93, +0.88] | 1 | Logs |
| ➖ | quality_gate_logs | % cpu utilization | -0.05 | [-2.97, +2.87] | 1 | Logs |
| ➖ | otel_to_otel_logs | ingress throughput | -0.21 | [-0.87, +0.44] | 1 | Logs |
| ➖ | quality_gate_idle_all_features | memory utilization | -0.54 | [-0.65, -0.42] | 1 | Logs bounds checks dashboard |
| ➖ | file_tree | memory utilization | -0.54 | [-0.66, -0.42] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency | egress throughput | -0.77 | [-1.55, +0.01] | 1 | Logs |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | links |
|---|---|---|---|---|
| ✅ | file_to_blackhole_0ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http1 | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http1 | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http2 | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http2 | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_1000ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_1000ms_latency_linear_load | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_100ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_100ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_300ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_300ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_500ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_500ms_latency | memory_usage | 10/10 | |
| ✅ | quality_gate_idle | memory_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_logs | lost_bytes | 10/10 | |
| ✅ | quality_gate_logs | memory_usage | 10/10 |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check lost_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
yuri-lipnesh
force-pushed
the
yuri_USMON-1124_ebpf_postgres_msg_counts
branch
from
0cd106b to
aed60c2
Compare
guyarb
requested changes
Nov 17, 2024
Comment on lines
213
to
215
| if (iteration_value->iteration >= POSTGRES_MAX_TAIL_CALLS_FOR_MAX_MESSAGES) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
why did you remove this check?
There was a problem hiding this comment.
Because I do not count iterations, I count total processed messages
There was a problem hiding this comment.
I'm not following why you removed the check, it is still a valid check, you might just need to modify the condition
There was a problem hiding this comment.
I've restored exit condition
github-actions
bot
added
long review
yuri-lipnesh
force-pushed
the
yuri_USMON-1124_ebpf_postgres_msg_counts
branch
from
50ecc18 to
d9c8ef6
Compare
guyarb
reviewed
Nov 21, 2024
yuri-lipnesh
force-pushed
the
yuri_USMON-1124_ebpf_postgres_msg_counts
branch
from
d9c8ef6 to
2044662
Compare
guyarb
reviewed
Nov 21, 2024
guyarb
reviewed
Nov 21, 2024
Comment on lines
274
to
276
| __u8 bucket_idx = ((__u8)(iteration_value->total_msg_count - PG_KERNEL_MSG_COUNT_FIRST_BUCKET) / PG_KERNEL_MSG_COUNT_BUCKET_SIZE) + 1; | ||
| bucket_idx = (bucket_idx >= PG_KERNEL_MSG_COUNT_NUM_BUCKETS) ? (PG_KERNEL_MSG_COUNT_NUM_BUCKETS - 1) : bucket_idx; | ||
| __sync_fetch_and_add(&pg_msg_counts->msg_count_buckets[bucket_idx], 1); |
There was a problem hiding this comment.
did you copy it from http2?
IIRC, in http2 we have a single function to handle all scenarios, I do recommend taking the same function and use it here
it will simplify the readability, and remove the if-else
There was a problem hiding this comment.
I replaced 'if's with ternary operator like http2 does
guyarb
reviewed
Nov 21, 2024
guyarb
reviewed
Dec 2, 2024
Comment on lines
32
to
44
| // Reads a message header from the given context. | ||
| // Returns the result of the read: successfully read header, end of packet reached, or fragmented message detected. | ||
| static __always_inline postgres_read_result_t read_message_header(pktbuf_t pkt, struct pg_message_header* header) { | ||
| u32 data_off = pktbuf_data_offset(pkt); | ||
| u32 data_end = pktbuf_data_end(pkt); | ||
| // Ensuring that the header is in the buffer. | ||
| if (data_off + sizeof(struct pg_message_header) > data_end) { | ||
| return false; | ||
| } | ||
| pktbuf_load_bytes(pkt, data_off, header, sizeof(struct pg_message_header)); | ||
| // Converting the header to host byte order. | ||
| header->message_len = bpf_ntohl(header->message_len); | ||
| return true; | ||
|
|
||
| if (data_off + sizeof(struct pg_message_header) <= data_end) { | ||
| // header fits within the buffer boundaries. | ||
| pktbuf_load_bytes(pkt, data_off, header, sizeof(struct pg_message_header)); | ||
| // Converting the header to host byte order. | ||
| header->message_len = bpf_ntohl(header->message_len); | ||
|
|
||
| return READ_OK; | ||
| } | ||
| if (data_off == data_end) { | ||
| return READ_END; | ||
| } | ||
| // the offset is outside the packet boundaries | ||
| return READ_FRAGMENTED; | ||
| } |
There was a problem hiding this comment.
Reporting READ_OK, READ_END and READ_FRAGMENTED does not bring any value to the PR.
Eventually, you're only checking if the result was READ_OK or not which is equivalent for the previous version of a boolean
There was a problem hiding this comment.
I increase counter pg_msg_counts->fragmented_packets when READ_FRAGMENTED and stop iterating tail calls when READ_END
There was a problem hiding this comment.
you're using it only once, and we can get the same outcome without modifying that internal function.
There was a problem hiding this comment.
I've restored previous read_message_header() function
guyarb
requested changes
Dec 2, 2024
Comment on lines
197
to
198
| kernelTel.reachedMaxMessages = libtelemetry.NewTLSAwareCounter(metricGroup, "messages_count_max_messages") | ||
| kernelTel.fragmentedPackets = libtelemetry.NewTLSAwareCounter(metricGroup, "messages_count_fragmented_packets") |
There was a problem hiding this comment.
the metric name is confusing. Please find a clear name for the purpose
There was a problem hiding this comment.
I've changed names
| kernelTel.fragmentedPackets = libtelemetry.NewTLSAwareCounter(metricGroup, "messages_count_fragmented_packets") | ||
|
|
||
| for i := range kernelTel.msgCountBuckets { | ||
| kernelTel.msgCountBuckets[i] = libtelemetry.NewTLSAwareCounter(metricGroup, "messages_count_bucket_"+(strconv.Itoa(i+1))) |
There was a problem hiding this comment.
Redundant parenthesis
Suggested change
| kernelTel.msgCountBuckets[i] = libtelemetry.NewTLSAwareCounter(metricGroup, "messages_count_bucket_"+(strconv.Itoa(i+1))) | |
| kernelTel.msgCountBuckets[i] = libtelemetry.NewTLSAwareCounter(metricGroup, "messages_count_bucket_"+strconv.Itoa(i+1)) |
| return kernelTel | ||
| } | ||
|
|
||
| // update the Postgres message counter store with new counters from the kernel, return immediately if nothing to add. |
There was a problem hiding this comment.
Suggested change
| // update the Postgres message counter store with new counters from the kernel, return immediately if nothing to add. | |
| // update the postgres message counter store with new counters from the kernel, return immediately if nothing to add. |
Comment on lines
220
to
221
| s := t.metricGroup.Summary() | ||
| log.Debugf("postgres kernel telemetry, isTLS=%t, summary: %s", isTLS, s) |
There was a problem hiding this comment.
Should be wrapped with ShouldLog. No need to generate the summary if the log level is above debug level
There was a problem hiding this comment.
I've added ShouldLog condition
Comment on lines
42
to
43
| typedef enum | ||
| { |
There was a problem hiding this comment.
Suggested change
| typedef enum | |
| { | |
| typedef enum { |
There was a problem hiding this comment.
I've removed this enum in recent commit
Comment on lines
274
to
275
| // save messages counter in bucket. | ||
| __u8 bucket_idx = iteration_value->total_msg_count; |
There was a problem hiding this comment.
bucket index (0-5) is initialized with total_msg_count?
Please use a variable with indicative name
There was a problem hiding this comment.
You copied the code from http2, but ignored the documentation we have there.
The code is not clear for the average reader and that's why we put the extensive documentation.
Also, it is better to move it into an helper function to make the code more readable
There was a problem hiding this comment.
I've created separate function update_msg_count_telemetry() to update telemetry buckets
Comment on lines
32
to
44
| // Reads a message header from the given context. | ||
| // Returns the result of the read: successfully read header, end of packet reached, or fragmented message detected. | ||
| static __always_inline postgres_read_result_t read_message_header(pktbuf_t pkt, struct pg_message_header* header) { | ||
| u32 data_off = pktbuf_data_offset(pkt); | ||
| u32 data_end = pktbuf_data_end(pkt); | ||
| // Ensuring that the header is in the buffer. | ||
| if (data_off + sizeof(struct pg_message_header) > data_end) { | ||
| return false; | ||
| } | ||
| pktbuf_load_bytes(pkt, data_off, header, sizeof(struct pg_message_header)); | ||
| // Converting the header to host byte order. | ||
| header->message_len = bpf_ntohl(header->message_len); | ||
| return true; | ||
|
|
||
| if (data_off + sizeof(struct pg_message_header) <= data_end) { | ||
| // header fits within the buffer boundaries. | ||
| pktbuf_load_bytes(pkt, data_off, header, sizeof(struct pg_message_header)); | ||
| // Converting the header to host byte order. | ||
| header->message_len = bpf_ntohl(header->message_len); | ||
|
|
||
| return READ_OK; | ||
| } | ||
| if (data_off == data_end) { | ||
| return READ_END; | ||
| } | ||
| // the offset is outside the packet boundaries | ||
| return READ_FRAGMENTED; | ||
| } |
There was a problem hiding this comment.
you're using it only once, and we can get the same outcome without modifying that internal function.
| utils.WaitForProgramsToBeTraced(t, consts.USMModuleName, GoTLSAttacherName, os.Getpid(), utils.ManualTracingFallbackEnabled) | ||
| } | ||
| pgClient := setupPGClient(t, serverAddress, isTLS) | ||
| defer func() { |
… extract counters telemetry.
…fragmented tcp packet.
…y direct call in testKernelMessagesCount()
…tgres_monitor_test.go
…postgres_monitor_test.go.
…kets(), replace 'require' by 'return' in assert block.
…eedingMax() replace 'require' by 'return' in assert block.
…t comment on exported KernelTelemetryMap
…instead of 'if' in handle_response()
…ate_msg_count_telemetry().
yuri-lipnesh
force-pushed
the
yuri_USMON-1124_ebpf_postgres_msg_counts
branch
from
d30ba7e to
5be261a
Compare
guyarb
requested changes
Dec 5, 2024
…bug logging from the update function.
guyarb
reviewed
Dec 8, 2024
…rnelTelemetry.Log().
…postgres_msg_counts
…postgres_msg_counts
Package size comparisonComparison with ancestor Diff per package
Decision✅ Passed |
guyarb
approved these changes
Dec 9, 2024
|
/merge |
Devflow running:
|
dd-mergequeue
bot
deleted the
yuri_USMON-1124_ebpf_postgres_msg_counts
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds Postgres message counters to the eBPF module of the system-probe
Motivation
PostgreSQL communication is done via a continuous stream of messages. This code gathers statistics on the number of Postgres messages processed by eBPF, providing valuable telemetry that enables optimization of eBPF buffer sizes for handling Postgres messages in the agent and system-probe processes.
Describe how to test/QA your changes
Passed unit tests for Postgres monitoring on local VM.
This branch will be tested on staging environment before merge.
Possible Drawbacks / Trade-offs
The complexity of an eBPF program will increase.
Additional Notes