Skip to content
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

fix: Add ddsource attribute to json-formatted logs #2727

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PROFeNoM
Copy link
Contributor

@PROFeNoM PROFeNoM commented Jun 20, 2024

Description

Problem: The PHP Log pipeline is not automatically used to process PHP logs ==> No Log Correlation by default, despite the trace identifiers being injected.

Two solutions:

  1. We inject the trace identifiers outside of context
  2. We use ddsource

Either way, it seems like we should have always been using ddsource + it is more elegant and is the actual way of doing it.

TBD - I gotta double test this on my sandbox. generating the artifact...

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@PROFeNoM PROFeNoM added 🐛 bug Something isn't working cat:log-injection labels Jun 20, 2024
@PROFeNoM PROFeNoM self-assigned this Jun 20, 2024
@pr-commenter
Copy link

pr-commenter bot commented Jun 20, 2024

Benchmarks

Benchmark execution time: 2024-06-20 15:00:03

Comparing candidate commit e1b1f33 in PR branch alex/fix/ddsource with baseline commit 3648fb5 in branch master.

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

scenario:PDOBench/benchPDOBaseline

  • 🟥 execution_time [+5.618µs; +13.686µs] or [+3.187%; +7.764%]

scenario:PDOBench/benchPDOBaseline-opcache

  • 🟥 execution_time [+3.641µs; +12.147µs] or [+2.075%; +6.924%]

scenario:PDOBench/benchPDOOverhead-opcache

  • 🟥 execution_time [+16.887µs; +18.821µs] or [+5.976%; +6.661%]

scenario:PDOBench/benchPDOOverheadWithDBM

  • 🟥 execution_time [+9.917µs; +13.735µs] or [+3.281%; +4.544%]

scenario:PDOBench/benchPDOOverheadWithDBM-opcache

  • 🟥 execution_time [+8.841µs; +16.218µs] or [+2.784%; +5.108%]

@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2024

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 65.96%. Comparing base (3648fb5) to head (17bd3f3).
Report is 8 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (3648fb5) and HEAD (17bd3f3). Click for more details.

HEAD has 3 uploads less than BASE
Flag BASE (3648fb5) HEAD (17bd3f3)
tracer-php 12 10
appsec-extension 1 0
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #2727       +/-   ##
=============================================
- Coverage     77.83%   65.96%   -11.88%     
  Complexity     2212     2212               
=============================================
  Files           227      201       -26     
  Lines         26604    22596     -4008     
  Branches        988        0      -988     
=============================================
- Hits          20708    14906     -5802     
- Misses         5370     7690     +2320     
+ Partials        526        0      -526     
Flag Coverage Δ
appsec-extension ?
tracer-extension 78.60% <ø> (+<0.01%) ⬆️
tracer-php 47.56% <0.00%> (-32.97%) ⬇️

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

Files Coverage Δ
src/DDTrace/Integrations/Logs/LogsIntegration.php 0.00% <0.00%> (-93.29%) ⬇️

... and 66 files with indirect coverage changes


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 3648fb5...17bd3f3. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working cat:log-injection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants