[exporterhelper] Preserve request span context in the persistent queue#13176
[exporterhelper] Preserve request span context in the persistent queue#13176dmitryax wants to merge 1 commit intoopen-telemetry:mainfrom
Conversation
dmitryax
requested review from
a team,
bogdandrutu,
dmathieu and
mx-psi
as code owners
dmitryax
force-pushed
the
context-marshaling-4
branch
from
1a52eb6 to
7a8a355
Compare
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (60.51%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #13176 +/- ##
==========================================
- Coverage 91.28% 91.28% -0.01%
==========================================
Files 510 512 +2
Lines 28725 28844 +119
==========================================
+ Hits 26223 26330 +107
- Misses 1988 1996 +8
- Partials 514 518 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dmitryax
force-pushed
the
context-marshaling-4
branch
from
7a8a355 to
bf58132
Compare
There was a problem hiding this comment.
I had to move the persistentqueue package to be able to call NewEncoder from exporterhelper
jmacd
left a comment
jmacd
left a comment
There was a problem hiding this comment.
I'm glad we're adding this feature.
My only regret is to see a new bespoke encoding be crafted here--I would prefer to see a human-readable (e.g.., JSON) or a protobuf-readable structure as opposed to a new magic number.
| featuregate.WithRegisterDescription("controls whether context should be stored alongside requests in the persistent queue"), | ||
| ) | ||
|
|
||
| type Encoding[T any] interface { |
There was a problem hiding this comment.
Alternative to this (not sure if this is a good idea) is to rely on the transform function and save the incoming context?
There was a problem hiding this comment.
@bogdandrutu, do you mean putting this information in the signal payload itself? Like under a special attribute resource attribute?
Another option can be having another protobuf wrapper specifically for the persistent queue
There was a problem hiding this comment.
in the "tracesRequest" you can store the context?
There was a problem hiding this comment.
I think is better to pass context to encoding though, if we are ok with this breaking change.
There was a problem hiding this comment.
ah yes, I investigated this approach with my initial PR but it seemed to make more sense to store it downstream of the individual request types (would need to be added to each request type including future signals)
jackgopack4
left a comment
jackgopack4
left a comment
There was a problem hiding this comment.
yes this makes sense especially using the libraries like otel/propagation
| # These lines will be padded with 2 spaces and then inserted directly into the document. | ||
| # Use pipe (|) for multiline entries. | ||
| subtext: | | ||
| Currently, it is behind the exporter.PersistRequestContext feature gate, which can be enabled by adding |
There was a problem hiding this comment.
since this one is the user chloggen, it may makes sense to add a comment as to what this enables (consistent internal telemetry/span links when using persistentqueue)
| req, err := re.requestEncoding.Unmarshal(b) | ||
| return context.Background(), req, err | ||
| } | ||
| ctx := tracePropagator.Extract(context.Background(), &bytesMapCarrier{bytesMap: bm}) |
There was a problem hiding this comment.
this propagation library is exactly what I should have used, will note going forward
There was a problem hiding this comment.
why separate file from encoding_test.go?
There was a problem hiding this comment.
It provides test utilities, not tests
| // Marshal is a function that can marshal a request into bytes. | ||
| Marshal(T) ([]byte, error) | ||
| // Marshal is a function that can marshal a request and its context into bytes. | ||
| Marshal(context.Context, T) ([]byte, error) |
There was a problem hiding this comment.
since exporterhelper is still beta version, it's not a problem modifying the API like this, right?
There was a problem hiding this comment.
Yes. This interface is marked as experimental
bogdandrutu
left a comment
bogdandrutu
left a comment
There was a problem hiding this comment.
LGTM, need a big rebase, but should get simpler this PR.
|
@bogdandrutu @jmacd here is another approach that doesn't involve the custom encoding but introduces new public module |
|
Closing in favor of #13188 |
4 participants
This change makes it possible to propagate span context through the Collector with enable persistent queue.
Currently, it is behind the exporter.PersistRequestContext feature gate, which can be enabled by adding
--feature-gates=exporter.PersistRequestContextto the collector command line. An exporter buffer stored by a previous version of the collector (or by a collector with the feature gate disabled) can be read by a newer collector with the feature enabled. However, the reverse is not supported: a buffer stored by a newer collector with the feature enabled cannot be read by an older collector (or by a collector with the feature gate disabled).Resolves #11740