Skip to content

Conversation

mfulb
Copy link
Contributor

@mfulb mfulb commented Mar 13, 2025

This PR corrects how the collector JSON for log forwarded labels handles log label key and values which have characters which must be escaped for JSON marshaling. The change is restricted to the daemon code to generate the log label JSON and does not impact the agent side of things.

The fix is to use the Go json module to handle the creation of the JSON. It will handle the escaping as needed. The array of label key/value pairs is converted to a Go map which is then marshaled into JSON. The case where either the label or value is empty (an invalid label) is handled so that these are not forwarded. This case should not occur as labels of this type should not be created by the agent but this check is added as additional protection against this case.

@mfulb mfulb requested review from zsistla and lavarou March 13, 2025 16:19
@newrelic-php-agent-bot
Copy link

newrelic-php-agent-bot commented Mar 13, 2025

Test Suite Status Result
Multiverse 8/8 passing
SOAK 79/79 passing

@codecov-commenter
Copy link

codecov-commenter commented Mar 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.87%. Comparing base (11a026f) to head (f0f4568).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #1035   +/-   ##
=======================================
  Coverage   77.87%   77.87%           
=======================================
  Files         198      198           
  Lines       27899    27899           
=======================================
  Hits        21727    21727           
  Misses       6172     6172           
Flag Coverage Δ
agent-for-php-7.2 78.13% <ø> (ø)
agent-for-php-7.3 78.15% <ø> (ø)
agent-for-php-7.4 77.86% <ø> (ø)
agent-for-php-8.0 77.25% <ø> (ø)
agent-for-php-8.1 77.65% <ø> (ø)
agent-for-php-8.2 ?
agent-for-php-8.3 77.26% <ø> (ø)
agent-for-php-8.4 77.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mfulb added 2 commits March 14, 2025 13:32
The previous code did not account for that labels can contain UTF-8 characters which
must be escaped.  This commit changes the code to use the Go marshalling for json
which will properly handle these characters.  A guard was added to make sure invalid
log labels are not converted to JSON (with an empty key or value).  The ingest code
should prevent this from happening but a secondary check here on JSON generation
is a reasonable safeguard.
@mfulb mfulb force-pushed the fix/log-labels-escape-chars branch from ca6a590 to f0f4568 Compare March 14, 2025 17:32
@mfulb mfulb merged commit 94421e7 into dev Mar 14, 2025
65 checks passed
@mfulb mfulb deleted the fix/log-labels-escape-chars branch March 14, 2025 19:17
@mfulb mfulb added this to the next-release milestone Mar 17, 2025
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

Successfully merging this pull request may close these issues.

4 participants