search

newrelic / newrelic-php-agent

The New Relic PHP Agent
https://opensource.newrelic.com/projects/newrelic/newrelic-php-agent
Apache License 2.0
119 stars 61 forks source link

feat(agent): optionally pad agent generated trace_ids #929

Closed ZNeumann closed 2 months ago

ZNeumann commented 3 months ago

W3C compliant headers will have a 32 char trace id. We generate these 32 character ids when we create a header by appending 0's in front of our 16 char trace id.

This PR takes that same concept, but optionally does so for when we store the trace id, not just when we are sending a header.

newrelic-php-agent-bot commented 3 months ago
Test Suite Status Result
Multiverse :white_check_mark: 7/7 passing
SOAK :x: 55/56 passing
codecov-commenter commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.13%. Comparing base (b4609db) to head (87d5da8). Report is 1 commits behind head on dev.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #929 +/- ## ======================================= Coverage 78.12% 78.13% ======================================= Files 194 194 Lines 26845 26857 +12 ======================================= + Hits 20972 20984 +12 Misses 5873 5873 ``` | [Flag](https://app.codecov.io/gh/newrelic/newrelic-php-agent/pull/929/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=newrelic) | Coverage Δ | | |---|---|---| | [agent-for-php-7.2](https://app.codecov.io/gh/newrelic/newrelic-php-agent/pull/929/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=newrelic) | `78.13% <100.00%> (+0.01%)` | :arrow_up: | | [agent-for-php-7.3](https://app.codecov.io/gh/newrelic/newrelic-php-agent/pull/929/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=newrelic) | `78.15% <100.00%> (+0.01%)` | :arrow_up: | | [agent-for-php-7.4](https://app.codecov.io/gh/newrelic/newrelic-php-agent/pull/929/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=newrelic) | `77.85% <100.00%> (+0.01%)` | :arrow_up: | | [agent-for-php-8.0](https://app.codecov.io/gh/newrelic/newrelic-php-agent/pull/929/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=newrelic) | `77.88% <100.00%> (+0.01%)` | :arrow_up: | | [agent-for-php-8.1](https://app.codecov.io/gh/newrelic/newrelic-php-agent/pull/929/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=newrelic) | `77.87% <100.00%> (+0.01%)` | :arrow_up: | | [agent-for-php-8.2](https://app.codecov.io/gh/newrelic/newrelic-php-agent/pull/929/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=newrelic) | `77.46% <100.00%> (+0.01%)` | :arrow_up: | | [agent-for-php-8.3](https://app.codecov.io/gh/newrelic/newrelic-php-agent/pull/929/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=newrelic) | `77.46% <100.00%> (+0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=newrelic#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ZNeumann commented 2 months ago

Does OTLP collector expect lowercase only letters in the trace id (similar to w3c traceparent header spec)?

All trace_ids that we create within the agent are lowercase, so this should not be important.

mfulb commented 2 months ago

Could we use get_linking_metadata() in some integration tests to verify the trace.id is 16 or 32 characters based on the setting of the INI value?

ZNeumann commented 2 months ago

Could we use get_linking_metadata() in some integration tests to verify the trace.id is 16 or 32 characters based on the setting of the INI value?

added in 5afcf9f

lavarou commented 2 months ago

Could tests for nr_distributed_trace_set_trace_id be added to axiom/tests/test_distributed_trace.c? There exist tests for nr_txn_create_w3c_traceparent_header in axiom/tests/test_txn.c which ensures nr_distributed_trace_create_w3c_traceparent_header padding works. But there don't exists explicit tests for nr_distributed_trace_set_trace_id padding covering 3 cases:

  • input is null
  • input is < 32
  • input is >= 32

Adressed in 87b7a85

lavarou commented 2 months ago

https://github.com/newrelic/newrelic-php-agent/pull/929#discussion_r1657754748

This probably needs its own test(s).