[Exporter.Geneva] Add thread-safety regression tests for MsgPackTraceExporter#3882
Conversation
github-actions
bot
added
the
comp:exporter.geneva
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3882 +/- ##
==========================================
- Coverage 71.59% 71.53% -0.07%
==========================================
Files 447 447
Lines 17827 17827
==========================================
- Hits 12764 12753 -11
- Misses 5063 5074 +11
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
mattsains
left a comment
mattsains
left a comment
There was a problem hiding this comment.
I'm glad we're adding this test, it's very needed. If you have time, we could do the same for the log exporter easily too!
|
|
||
| // Validate: the serialized data must be valid MessagePack | ||
| // representing a proper Fluentd Forward Mode message. | ||
| // Under the race condition, the Map16 header field count |
There was a problem hiding this comment.
I get that this test is a reaction to the bug, but this is the fifth time in this file that the bug is explicitly called out - is that necessary? This test will live on as a general test of multi-threading support, not simply that the exact bug we identified won't come back. In my view, the file inappropriately refers back to the specific issue that caused us to realise the need for this test throughout, and these references should mostly be removed
| // In the buggy code, repeated calls can cause prepopulatedFields to grow | ||
| // (duplicate entries added from resource attributes), making the Count | ||
| // diverge from what's in the prologue. | ||
| exporter.CreateFraming(); |
There was a problem hiding this comment.
I don't think it makes sense to call CreateFraming in the tests. Perhaps CreateFraming should be changed to be private.
This test does not do what it says it does - CreateFraming is called twice, but synchronously, so it just does the same thing twice. I think this test should be removed.
| using var activitySource = new ActivitySource("ThreadSafetyTest"); | ||
| using var listener = new ActivityListener | ||
| { | ||
| ShouldListenTo = _ => true, |
There was a problem hiding this comment.
The other tests following a convention of only listening to the specific activity source name of the test, and comments say this is to avoid interference with other tests. However, I am not sure this is actually possible because the listener will not be attached to any other tests' activity sources. Anyway, not sure what to suggest here but just pointing out it's different to the other tests' layout.
| var data = new byte[serialized.Count]; | ||
| Array.Copy(serialized.Array!, serialized.Offset, data, 0, serialized.Count); | ||
|
|
||
| var deserialized = MessagePack.MessagePackSerializer.Deserialize<object>( |
There was a problem hiding this comment.
To improve this test, you can manually construct a MessagePackReader, and make sure that all the bytes have been consumed by the deserialize call. This way we ensure that there are no extra message pack encoded fields that were left off by an incorrect field count. That would also let you remove the test called CreateFraming_CalledMultipleTimes_FieldCountRemainsConsistent
| Assert.Contains("env_cloud_role", mapping.Keys); | ||
| Assert.Equal("TestService", mapping["env_cloud_role"]); | ||
| Assert.Contains("env_cloud_roleInstance", mapping.Keys); | ||
| Assert.Equal("Instance123", mapping["env_cloud_roleInstance"]); |
There was a problem hiding this comment.
I suggest asserting the number of fields in mapping to ensure there are no extra, unintended fields
| /// Under the race condition, different threads can end up with different field counts. | ||
| /// </summary> | ||
| [Fact] | ||
| public void SerializeActivity_DifferentThreads_SameFieldCount() |
There was a problem hiding this comment.
see my comment above discussing a way to roll this test into SerializeActivity_ConcurrentThreads_ProducesValidMsgPack
3 participants
Adds regression tests that verify
MsgPackTraceExporter.SerializeActivityproduces valid MessagePack output under concurrent multi-threaded access.PR #3214 introduced a race condition where multiple threads calling
SerializeActivitysimultaneously would each invokeCreateFraming(), concurrently mutating the sharedprepopulatedFieldsdictionary. This causedDictionaryinternal state corruption and resulted in "Bad forward protocol format" errors on the receiving end.The fix was delivered in #3881, but no regression tests existed to cover the
multi-threaded scenario. These tests fill that gap.
Tests added
SerializeActivity_ConcurrentThreads_ProducesValidMsgPack— 8 threadsserialize activities concurrently and validate that every output is a well-formed
Fluentd Forward Mode MessagePack message.
CreateFraming_CalledMultipleTimes_FieldCountRemainsConsistent— CallsCreateFraming()multiple times and asserts thatprepopulatedFields.Countand
bufferPrologue.Lengthremain stable.SerializeActivity_DifferentThreads_SameFieldCount— 8 threads serializeactivities concurrently and verify the Map16 field count is identical across
all threads.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes