-
Notifications
You must be signed in to change notification settings - Fork 398
Telemetry: fix reporting of profiling and appsec state #5075
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
base: master
Are you sure you want to change the base?
Conversation
|
Thank you for updating Change log entry section 👏 Visited at: 2025-11-20 17:51:39 UTC |
Typing analysisThis PR does not change typing compared to the base branch. |
41c5624 to
1c682fd
Compare
BenchmarksBenchmark execution time: 2025-12-02 15:43:20 Comparing candidate commit 1fc3d9f in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 43 metrics, 2 unstable metrics. scenario:profiling - Allocations ()
|
1c682fd to
6c67f9e
Compare
Strech
left a comment
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.
I still have gaps in understanding of the life-cycle of our components and especially telemetry, but looks good to me (I would recommend waiting for someone else feedback too).
Maybe few improvements over typespecs
|
So this PR is on top of #5074 and most of the diff actually was (and was reviewed) in that PR. |
8b4b7b1 to
b5f85eb
Compare
b5f85eb to
1fc3d9f
Compare
What does this PR do?
Fixes telemetry reporting of profiling and appsec enabled/disabled state (in app-started events).
Profiler was reported as not running in the app-started event regardless of actual state, due to the way enablement check was implemented - it looked at whether profiler was started, not just enabled, and profiler was started after telemetry by the component tree.
This PR changes the check to consider whether the Profiler instance exists, which should indicate whether profiling is actually enabled and working.
AppSec was reported based on its configuration, not actual state. If AppSec was requested to be enabled but failed prerequisites (Ruby version/FFI version) or failed to initialize, it would still be reported as enabled.
We have the configuration for each component sent separately which contains the requested enabled/disabled values, therefore the product field should have the actual state.
I took the liberty to change
AppSec.enabled?to read component state also.Other, smaller changes while I was looking at the code:
Telemetry is now shut down last, to permit all other code to send diagnostics during shutdown. Previously process discovery was shut down last, it didn't send anything via telemetry that I saw, but I moved it ahead of telemetry.
Motivation:
Code review in #5067
Change log entry
None
Additional Notes:
AppSec does not store the settings into its component, therefore all configuration is not necessarily correct (during reconfiguration, existing components will report/use new configuration rather than "their" configuration). This most notably makes
rasp_enabled?code weird in this PR, but applies to all of AppSec configuration from a quick look.AppSec initialization block did not report the exception it rescued, I changed it to do so (the tests I wrote didn't report any reason why AppSec failed to initialize without this change).
Only profiling reports the reason why it wasn't enabled, DI and AppSec do not.
How to test the change?
Integration tests added to verify both configured enabled/disabled state and actual enabled/disabled state for all products reported to telemetry (AppSec, DI, profiling).