fix(plugin): OTEL exporting with translate #10332

StarlightIbuki · 2023-02-21T09:09:27Z

Summary

We represent trace spans with a universal format for all kinds of mainstream tracers and translate them to different formats when propagating headers and exporting traces. However, OTEL misses the handling of trace ID length.

Checklist

The Pull Request has tests
There's an entry in the CHANGELOG
N/A There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix KAG-652

CHANGELOG.md

flrgh · 2023-02-23T00:09:21Z

I've updated the PR with a couple fixes. I think it meets the correct requirements now, but there are some warts:

We only have test coverage for the trace ID zero-pad case. There is no test for the trace ID truncate case, because an incoming trace ID that is too long will be discarded as invalid.
The way we are copying the span table seems like it may have unintended consequences. Spans use table pools under the hood, so the PDK expects to manage their lifecycle. Copying spans outside of the PDK can therefore lead to situations where tables are orphaned and gc-ed instead of being returned to the pool as intended.

StarlightIbuki · 2023-02-23T03:00:23Z

I've updated the PR with a couple fixes. I think it meets the correct requirements now, but there are some warts:

We only have test coverage for the trace ID zero-pad case. There is no test for the trace ID truncate case, because an incoming trace ID that is too long will be discarded as invalid.

The way we are copying the span table seems like it may have unintended consequences. Spans use table pools under the hood, so the PDK expects to manage their lifecycle. Copying spans outside of the PDK can therefore lead to situations where tables are orphaned and gc-ed instead of being returned to the pool as intended.

Answering question 2: the copied span won't survive the request so it will be GC-ed, and its reference to the members will also be gone. So the only reference to those members should be from the original span, which will be returned to the pool. If the PDK decides to also reuse the members, it will still be fine as the table returned to the pool has reference to the members.

PS: table pool is just referencing returned tables to prevent them from GC-ed and has no special mechanism, thus we cannot "GC a table that is supposed to be returned to the pool". The worst situation is that we returned tables that are used somewhere else unaware of them being recycled which cannot happen here.

kong/plugins/opentelemetry/otlp.lua

StarlightIbuki · 2023-02-23T03:09:05Z

I've updated the PR with a couple fixes. I think it meets the correct requirements now, but there are some warts:

We only have test coverage for the trace ID zero-pad case. There is no test for the trace ID truncate case, because an incoming trace ID that is too long will be discarded as invalid.

The way we are copying the span table seems like it may have unintended consequences. Spans use table pools under the hood, so the PDK expects to manage their lifecycle. Copying spans outside of the PDK can therefore lead to situations where tables are orphaned and gc-ed instead of being returned to the pool as intended.

To answer question 1: I agree that we need a test for the truncating case. But the code you're referring to (handling to OT header) does not mean truncating doesn't work. We're translating traces, and a hypothetical trace format may have a valid trace ID larger than that.

StarlightIbuki · 2023-02-23T05:13:54Z

However there's not a type of ID that is long enough to go through the truncating path, so we skip it for now.

We represent trace spans with a universal format for all kinds of mainstream tracers and translate them to different formats when propagating headers and exporting traces. However, OTEL misses the handling of trace ID length. Fix KAG-652

CHANGELOG.md

kong/plugins/opentelemetry/otlp.lua

Co-authored-by: Chrono <[email protected]>

chronolaw

We could do more code clean for it, but I think it is OK now.

kikito · 2023-02-23T09:01:05Z

kong/plugins/opentelemetry/otlp.lua

@@ -72,9 +77,39 @@ local function transform_events(events)
  return pb_events
 end

+-- this function is to translate the span to otlp span, but of universal span format


I don't understand what "but of universal span format" means.

The original design to support arbitrary types of tracing plugins has not made it clear:

We have an internal representation for span, and it should preserve what it received from downstream requests at best effort;

when exporting the data to collectors, it needs to transform the span to meet the requirement with its field (DD, OTEL, etc), and then translate it into a format used for the reporting API (gRPC, JSONRPC, etc). This function does the first step but not the second one.

kikito · 2023-02-23T09:03:57Z

kong/plugins/opentelemetry/otlp.lua

+  span = translated
+
+  return span


Nitpick, not needed if we don't do a shallow copy.

Suggested change

span = translated

return span

return translated

kikito · 2023-02-23T09:08:27Z

kong/plugins/opentelemetry/otlp.lua

+    new_id = NULL:rep(TRACE_ID_LEN - len) .. trace_id
+  end
+
+  local translated = shallow_copy(span)


Why is a shallow_copy needed?

I would have expected this function to do something like:

local function adjust_trace_id_for_oltp(trace_id)

And the usage would be, when creating the root span:

root_span.trace_id = adjust_trace_id_for_oltp(trace_id)

And inside transform_span:

local pb_span = { trace_id = adjust_trace_id_for_oltp(span.trace_id),

Also, I don't recommend using abstract names (like transform_span or translate_span) when the function is only doing one modification on one field. The tradeoff ("making the code easier to modify in the future if we find that we need to do more modifications in the future") is not worth the upfront cost ("making the code more difficult to read now"). Future us can always group several modifications into one if the need arises.

Why is a shallow_copy needed?

The span passed in may also be used by other plugins. We need to preserve as much original information as possible, and only do the transformation for exporting/propagating.

I agree naming is a problem, but transform_span is used as some kind of interface for tracing plugins (we can see a function with the same name from datadog-tracing).
I'm adding comments and renaming the internal function.

And the function is in the file "otlp.lua" so I think we do not need to suffix it with "_otlp".

And the usage would be, when creating the root span:

We cannot just modify the root_span given the reason: https://github.com/Kong/kong/pull/10332/files#r1115411177

Co-authored-by: Chrono <[email protected]> Co-authored-by: Michael Martin <[email protected]> (cherry picked from commit 6fa55b3)

Co-authored-by: Chrono <[email protected]> Co-authored-by: Michael Martin <[email protected]> (cherry picked from commit 6fa55b3) Co-authored-by: Xumin <[email protected]> Co-authored-by: Enrique García Cota <[email protected]>

pull-request-size bot added the size/M label Feb 21, 2023

github-actions bot added the plugins/opentelemetry label Feb 21, 2023

StarlightIbuki force-pushed the fix/otel-trace-id-len branch from b2ac59b to 2e85bd2 Compare February 21, 2023 09:15

github-actions bot added the changelog label Feb 21, 2023

StarlightIbuki commented Feb 21, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

RobSerafini added this to the 3.2.1 milestone Feb 22, 2023

tyler-ball added the backport release/3.2.x label Feb 22, 2023

dndx requested a review from chronolaw February 23, 2023 02:55

chronolaw reviewed Feb 23, 2023

View reviewed changes

kong/plugins/opentelemetry/otlp.lua Outdated Show resolved Hide resolved

StarlightIbuki and others added 7 commits February 23, 2023 13:14

fix(plugin): OTEL exporting with translate

abb3cf6

We represent trace spans with a universal format for all kinds of mainstream tracers and translate them to different formats when propagating headers and exporting traces. However, OTEL misses the handling of trace ID length. Fix KAG-652

Update CHANGELOG.md

db0d403

shallow copy to prevent infinite recusive

2065cfc

fix(plugin): copy span metatable

9ce8c63

tests(plugin): update test case for trace id padding

ce60216

perf(plugin): avoid needless copying of span

bde853a

apply suggestion

4aba447

StarlightIbuki force-pushed the fix/otel-trace-id-len branch from 93d3168 to 4aba447 Compare February 23, 2023 05:14

chronolaw reviewed Feb 23, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

chronolaw reviewed Feb 23, 2023

View reviewed changes

kong/plugins/opentelemetry/otlp.lua Show resolved Hide resolved

StarlightIbuki and others added 3 commits February 23, 2023 14:23

Update CHANGELOG.md

331415d

Co-authored-by: Chrono <[email protected]>

apply suggestion

dc3813d

no need to check equility

610bafb

chronolaw approved these changes Feb 23, 2023

View reviewed changes

kikito reviewed Feb 23, 2023

View reviewed changes

naming&explaination

a5a8008

kikito approved these changes Feb 23, 2023

View reviewed changes

kikito merged commit 6fa55b3 into master Feb 23, 2023

kikito deleted the fix/otel-trace-id-len branch February 23, 2023 15:35

mashapedeployment mentioned this pull request Feb 23, 2023

[Backport release/3.2.x] fix(plugin): OTEL exporting with translate #10358

Merged

mashapedeployment pushed a commit that referenced this pull request Feb 23, 2023

fix(plugin): OTEL exporting with translate (#10332)

0f3ab6d

Co-authored-by: Chrono <[email protected]> Co-authored-by: Michael Martin <[email protected]> (cherry picked from commit 6fa55b3)

tyler-ball removed this from the 3.2.1 milestone Feb 23, 2023

nbialostosky mentioned this pull request Mar 2, 2023

Allow w3c to be set as the default propagation for opentelemetry plugin #10246

Closed

1 task

This was referenced Mar 16, 2023

release/3.2.x #10505

Closed

release/3.2.x #10506

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(plugin): OTEL exporting with translate #10332

fix(plugin): OTEL exporting with translate #10332

StarlightIbuki commented Feb 21, 2023 •

edited

Loading

flrgh commented Feb 23, 2023

StarlightIbuki commented Feb 23, 2023 •

edited

Loading

StarlightIbuki commented Feb 23, 2023

StarlightIbuki commented Feb 23, 2023

chronolaw left a comment

kikito Feb 23, 2023

StarlightIbuki Feb 23, 2023

kikito Feb 23, 2023

kikito Feb 23, 2023

kikito Feb 23, 2023

StarlightIbuki Feb 23, 2023

StarlightIbuki Feb 23, 2023

StarlightIbuki Feb 23, 2023

StarlightIbuki Feb 23, 2023

fix(plugin): OTEL exporting with translate #10332

fix(plugin): OTEL exporting with translate #10332

Conversation

StarlightIbuki commented Feb 21, 2023 • edited Loading

Summary

Checklist

Issue reference

flrgh commented Feb 23, 2023

StarlightIbuki commented Feb 23, 2023 • edited Loading

StarlightIbuki commented Feb 23, 2023

StarlightIbuki commented Feb 23, 2023

chronolaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StarlightIbuki commented Feb 21, 2023 •

edited

Loading

StarlightIbuki commented Feb 23, 2023 •

edited

Loading