fix(daemon): aggregate packages throughout harvest #1103
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1103 +/- ##
==========================================
- Coverage 78.12% 77.95% -0.17%
==========================================
Files 193 192 -1
Lines 27989 27792 -197
==========================================
- Hits 21866 21666 -200
- Misses 6123 6126 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if pkg.Name == expectedPackages[i].Name { | ||
| //fmt.Printf("Match - index = %d\n", j) |
There was a problem hiding this comment.
Were all of these comments left in previously for ease of future debugging efforts?
There was a problem hiding this comment.
I don't know the origin of these comments.
There was a problem hiding this comment.
Did the autoformatter remove them?
There was a problem hiding this comment.
I did while I was debugging a problem with the tests
|
|
||
| packages.data = append(packages.data, PhpPackagesKey{name, version}) | ||
| } | ||
|
|
||
| packages.numSeen = 1 |
There was a problem hiding this comment.
I'm not sure what this field does any more - but if we keep it should it be incremented by the number of packages created in the for loop above?
There was a problem hiding this comment.
I'm also interested in how this gets resolved. See related comment: https://github.com/newrelic/newrelic-php-agent/pull/1103/files#r2261468915
bduranleau-nr
force-pushed
the
fix/vm-package-aggregation
branch
from
ac27f9a to
59cf8f6
Compare
| // NumSaved returns the total number PHP packages payloads stored. | ||
| // Should always be 0 or 1. The agent reports all the PHP |
There was a problem hiding this comment.
Is "Should always be 0 or 1" still valid? Now that daemon potentially holds on to multiple php packages payloads received from the agent, shouldn't this represent the number of times the daemon received php packages payload from the agent in a given harvest cycle?
There was a problem hiding this comment.
As far as I can tell this value isn't sent to the backend or used anywhere other than the conditional checks as an indicator that packages have been set for the current harvest. I don't believe it is used in any kind of metrics or instrumentation data, so maybe we should consider changing it entirely to a bool. I think this was added as an artifact of the other Event types that report seen vs saved, but it doesn't really fit cleanly into the packages VM logic.
There was a problem hiding this comment.
At least the comment should be updated because it is misleading.
The addition of packages.filteredData was causing the processor_test to hang, due to packages.filteredData only being set during the harvest process. The harvest.empty check would fail, and the harvest would never occur, causing the test to hang indefinitely
since the final result of the package data is no longer in the PhpPackages.data field, add an app instance to execute the filterPhpPackages logic and set the result == pkg.filteredData. This will allow the `CollectorJSON` function to run successfully and return the expected result.
Refactor package filtering logic. Previously filtering was owned by the App module. This caused depedency problems when trying to invoke filtering (necessary for final json generation) outside of the app context. Moving ownership to the PhpPackages module allows access to filtering whenever relevant. Also refactored ownership of JSON generation. Package JSON generation is no longer split between the filter method and the CollectorJSON method and is instead fully owned by CollectorJSON.
consolidate the SetPhpPackages method into the AddPhpPackagesFromData method. Instead of storing the data as json, unmarshal it and store the raw data in the `data` slice as a PhpPackagesKey struct. Reject bad data immediately instead of allowing it to persist in the data array.
bduranleau-nr
force-pushed
the
fix/vm-package-aggregation
branch
from
906481a to
76c2044
Compare
Fixes a bug in the daemon behavior where only the last VM package data payload from the agent was harvested by the daemon. See this line to better understand the general behavior at play:
newrelic-php-agent/daemon/internal/newrelic/php_packages.go
Line 57 in 765888d
This issue was exposed by SOAK testing the composer VM once-per-process package detection, where the composer package data was being overridden by the legacy VM data. The fix required a refactor to how the daemon stores and processes package data, while still observing the need to de-duplicate data sent to the
update_loaded_modulesendpoint for processing.The
filterPhpPackageslogic was removed fromAppand placed under thePhpPackagesstruct as the newFilterfunction.The former
dataJSONStringfield has been converted into aPhpPackagesKeysliceto store all of the package data sent by the agent for a single harvest cycle. Data received from the agent is no longer stored as JSON, but unmarshalled and verified before the daemon accepts it and stores it as aPhpPackagesKeyslicewith the raw name and version. Invalid package data received from the agent will no longer be stored indataby default and is instead discarded. To store the filtered package data, a newfilteredPkgsPhpPackagesKeyslicefield has been added to thePhpPackagesstruct.The responsibility of marshalling the final json payload for the backend endpoint has been consolidated into the
CollectorJSONfunction for simpler data handling, and better accessibility for testing.SetPhpPackageshas been removed and logic consolidated into the parent caller,AddPhpPackagesFromData.