Add traceId to documentId used for spans#5684
Add traceId to documentId used for spans#5684KarstenSchnitter merged 2 commits intoopensearch-project:mainfrom
Conversation
adresses opensearch-project#5370 OpenTelemetry spans are currently indexed using the spanId as documentId. This can lead to collisions where spans are not indexed or overwrite each other. This change adds the traceId to the documentId. OpenTelemetry assumes the combination of traceId and spanId to be unique. If there is still a collision it is safe to assume, that it occurs because of a resending of a previously indexed span. Signed-off-by: Karsten Schnitter <k.schnitter@sap.com>
|
I manually checked with the |
Thanks @KarstenSchnitter ! Probably the best test would be one that replicates the existing issue. For example, send two spans with the same spanId, but different traceIds. Then verify they are both there. This should help ensure that no other code paths could lead to this bug recurring. |
|
@dlvenable I implemented the suggested test case. Thanks for the feedback. |
|
@KarstenSchnitter the change looks good to me but would it break the existing behavior? Should we add a new index type instead? |
|
@kkondaka this change avoids conflicts in document ids caused by collisions in span ids in different traces. I think this is a bug. If you used the existing behaviour to update a span, you can still do it, since the update would contain the same trace id and hence the document id is stable. I think, introducing a new index type for this change might be more confusing than helpful. |
|
I am looking in to the failing integrations tests. |
Adds an integration tests, that indexes two spans with the same span id but different trace ids. With the old implementation the two spans would overwrite each other. This tests checks, that two spans are returned by the query. This test is only green with the new document id implementation and fails with the earlier. Signed-off-by: Karsten Schnitter <k.schnitter@sap.com>
58b41b6 to
2db3dd0
Compare
|
@dlvenable or @kkondaka I cannot get the last test to pass. But I think it is unrealted to my change. Can you suggest how to proceed? |
I lost track of this change. I approved it. That last test is flaky, so we can ignore. |
59705ae
into
opensearch-project:main
* Add traceId to documentId used for spans adresses opensearch-project#5370 OpenTelemetry spans are currently indexed using the spanId as documentId. This can lead to collisions where spans are not indexed or overwrite each other. This change adds the traceId to the documentId. OpenTelemetry assumes the combination of traceId and spanId to be unique. If there is still a collision it is safe to assume, that it occurs because of a resending of a previously indexed span. Signed-off-by: Karsten Schnitter <k.schnitter@sap.com> * Add integration test forcing the change Adds an integration tests, that indexes two spans with the same span id but different trace ids. With the old implementation the two spans would overwrite each other. This tests checks, that two spans are returned by the query. This test is only green with the new document id implementation and fails with the earlier. Signed-off-by: Karsten Schnitter <k.schnitter@sap.com> --------- Signed-off-by: Karsten Schnitter <k.schnitter@sap.com> Signed-off-by: Jonah Calvo <caljonah@amazon.com>
Description
OpenTelemetry spans are currently indexed using the spanId as documentId. This can lead to collisions where spans are not indexed or overwrite each other.
This change adds the traceId to the documentId. OpenTelemetry assumes the combination of traceId and spanId to be unique. If there is still a collision it is safe to assume, that it occurs because of a resending of a previously indexed span.
Issues Resolved
Resolves #5370
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.