Skip to content

Ensure the msgpack contents include only valid utf8#2698

Merged
bwoebi merged 3 commits intomasterfrom
bob/fix-utf8-msgpack
Jun 8, 2024
Merged

Ensure the msgpack contents include only valid utf8#2698
bwoebi merged 3 commits intomasterfrom
bob/fix-utf8-msgpack

Conversation

@bwoebi
Copy link
Copy Markdown
Collaborator

@bwoebi bwoebi commented Jun 8, 2024

Ensure no utf8 is contained in msgpack strings.

The agent would properly sanitize this away, but the sidecar is not. All traces containing a single invalid utf-8 sequence were dropped.

@bwoebi bwoebi requested review from a team as code owners June 8, 2024 15:55
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 8, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.

Project coverage is 77.96%. Comparing base (c5f2dcb) to head (f91f0db).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2698      +/-   ##
============================================
- Coverage     77.96%   77.96%   -0.01%     
  Complexity     2212     2212              
============================================
  Files           227      227              
  Lines         26551    26559       +8     
  Branches        988      988              
============================================
+ Hits          20701    20707       +6     
- Misses         5324     5326       +2     
  Partials        526      526
Flag Coverage Δ
appsec-extension 69.13% <ø> (ø)
tracer-extension 78.85% <72.72%> (-0.01%) ⬇️
tracer-php 80.52% <ø> (ø)

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

Files Coverage Δ
ext/serializer.c 82.12% <72.72%> (-0.06%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5f2dcb...f91f0db. Read the comment docs.

@pr-commenter
Copy link
Copy Markdown

pr-commenter Bot commented Jun 8, 2024

Benchmarks

Benchmark execution time: 2024-06-08 16:21:37

Comparing candidate commit f91f0db in PR branch bob/fix-utf8-msgpack with baseline commit c5f2dcb in branch master.

Found 3 performance improvements and 5 performance regressions! Performance is the same for 170 metrics, 0 unstable metrics.

scenario:MessagePackSerializationBench/benchMessagePackSerialization

  • 🟥 execution_time [+74.329µs; +77.611µs] or [+50.151%; +52.366%]

scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache

  • 🟥 execution_time [+75.011µs; +76.889µs] or [+50.639%; +51.906%]

scenario:PDOBench/benchPDOBaseline-opcache

  • 🟥 execution_time [+14.951µs; +17.647µs] or [+8.600%; +10.151%]

scenario:PDOBench/benchPDOOverhead

  • 🟥 execution_time [+15.505µs; +18.772µs] or [+5.613%; +6.796%]

scenario:PDOBench/benchPDOOverhead-opcache

  • 🟥 execution_time [+5.792µs; +14.535µs] or [+2.007%; +5.037%]

scenario:SamplingRuleMatchingBench/benchRegexMatching1

  • 🟩 execution_time [-457.792ns; -184.608ns] or [-6.588%; -2.657%]

scenario:SamplingRuleMatchingBench/benchRegexMatching2

  • 🟩 execution_time [-398.199ns; -152.601ns] or [-5.782%; -2.216%]

scenario:SamplingRuleMatchingBench/benchRegexMatching3

  • 🟩 execution_time [-534.659ns; -284.541ns] or [-7.689%; -4.092%]

pierotibou
pierotibou reviewed Jun 8, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@pierotibou pierotibou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get why this would be an issue when going through the agent (as you said it strips utf8).
And if this is for agentless, I don't think we should change the C code. At least short term, we envision the exporter to take care of all that kind of things, so that the tracers keep a consistent implementation

Comment thread ext/serializer.c Outdated
Comment thread ext/serializer.c Outdated
pierotibou
pierotibou approved these changes Jun 8, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@pierotibou pierotibou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool thanks for the offline explanations and for adding the change behind the sidecar flag.

@bwoebi bwoebi merged commit 8d07c87 into master Jun 8, 2024
@bwoebi bwoebi deleted the bob/fix-utf8-msgpack branch June 8, 2024 20:20
@github-actions github-actions Bot added this to the 1.1.0 milestone Jun 8, 2024
@pierotibou pierotibou mentioned this pull request Jun 10, 2024
Closed
2 tasks
morrisonlevi pushed a commit that referenced this pull request Jun 10, 2024
@bwoebi @pierotibou @morrisonlevi
Ensure the msgpack contents include only valid utf8 (#2698)
a540cb1 
* Ensure the msgpack contents include only valid utf8

Signed-off-by: Bob Weinand <[email protected]>

* Update ext/serializer.c

Co-authored-by: Pierre Bonet <[email protected]>

* Avoid minor perf impact on msgpack serialization with sidecar sender disabled

Signed-off-by: Bob Weinand <[email protected]>

---------

Signed-off-by: Bob Weinand <[email protected]>
Co-authored-by: Pierre Bonet <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pierotibou pierotibou pierotibou approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.1.0

Development

Successfully merging this pull request may close these issues.

3 participants

@bwoebi @codecov-commenter @pierotibou