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 linking with newrelic.distributed_tracing_enabled=1 #3

Merged
merged 1 commit into from
May 30, 2024

Conversation

ostrolucky
Copy link
Contributor

There are several behavioural differences with newrelic.distributed_tracing_enabled=1 that this patch addresses

  1. newrelic_add_custom_parameter('traceId', $value); hack does not work. Traces do not get traceId matching $value assigned
  2. As an obvious solution, custom TraceIdFactory which uses newrelic_get_trace_metadata instead of UUIDs presents itself. However, this also does not work, because:
  3. Transaction ID apparently might change at any point of time during the request. Specifically, I've observed newrelic_name_transaction changes this ID.

This bundle in sequence:

  1. creates TraceID at beginning of request
  2. calls newrelic_name_transaction
  3. calls newrelic_add_custom_parameter('traceId', $this->traceId->__toString());

This doesn't work, because step 3 uses ID generated at step 1, but NR changed ID in step 2, hence Logs get different trace.id assigned than NR Transaction has.

This could be solved by custom NewrelicNativeTraceIdFactory + calling $this->traceId->refresh() right after $this->interactor->setApplicationName( but I worry that this might work in this case, but there will be other cases where NR extension changes current traceId.

Hence, I think solution in this commit is quite robust and will work with both states of newrelic.distributed_tracing_enabled out of the box. Only disadvantage is that if users have newrelic.distributed_tracing_enabled=1, even if they define custom traceIdFactory, "native" NR transaction ID will be used despite whatever they generate in their custom factory. However, I believe this is OK, because whatever they generate in their custom factory wouldn't be taken over by NR extension anyways, because of very first point mentioned in this commit message

There are several behavioural differences with `newrelic.distributed_tracing_enabled=1` that this patch addresses

1. `newrelic_add_custom_parameter('traceId', $value);` hack does not work. Traces do not get traceId matching $value assigned
2. As an obvious solution, custom TraceIdFactory which uses `newrelic_get_trace_metadata` instead of UUIDs presents itself. However, this also does not work, because:
3. Transaction ID apparently might change at any point of time during the request. Specifically, I've observed `newrelic_name_transaction` changes this ID.

This bundle in sequence:
 1. creates TraceID at beginning of request
 2. calls `newrelic_name_transaction`
 3. calls `newrelic_add_custom_parameter('traceId', $this->traceId->__toString());`

 This doesn't work, because step 3 uses ID generated at step 1, but NR changed ID in step 2, hence Logs get different `trace.id` assigned than NR Transaction has.

 This _could_ be solved by custom NewrelicNativeTraceIdFactory + calling `$this->traceId->refresh()` right after `$this->interactor->setApplicationName(` but I worry that this might work in this case, but there will be other cases where NR extension changes current traceId.

 Hence, I think solution in this commit is quite robust and will work with both states of `newrelic.distributed_tracing_enabled` out of the box. Only disadvantage is that if users have `newrelic.distributed_tracing_enabled=1`, even if they define custom traceIdFactory, "native" NR transaction ID will be used despite whatever they generate in their custom factory. However, I believe this is OK, because whatever they generate in their custom factory wouldn't be taken over by NR extension anyways, because of very first point mentioned in this commit message
@rguennichi rguennichi merged commit c2c9eb9 into CHECK24-CP:main May 30, 2024
2 checks passed
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.

2 participants