Skip to content
This repository was archived by the owner on Apr 2, 2024. It is now read-only.

Conversation

arajkumar
Copy link
Member

@arajkumar arajkumar commented Oct 6, 2022

Description

This commit reuses fixtures from upstream jaeger[1] which is exposed as golang pkg.

Though we use non release version v1.38.2-0.20221006002917-5bf8a28fe06d, it doesn't have any other changes except [1] which is safe.

This PR also fixes TestJaegerStorageIntegration/*/GetOperations test which was failing due to a typo.

Note: Currently we exclude FindTraces/Tags_+_Operation_name test which is still failing after merging both #1681 and #1678. New bug has been created to address the same.

[1] jaegertracing/jaeger#3944

Merge requirements

Please take into account the following non-code changes that you may need to make with your PR:

  • CHANGELOG entry for user-facing changes
  • Updated the relevant documentation

@arajkumar arajkumar requested review from a team as code owners October 6, 2022 08:13
@arajkumar arajkumar marked this pull request as draft October 6, 2022 08:23
@arajkumar arajkumar force-pushed the jaeger-storage-int-tests branch 2 times, most recently from 9b4f017 to ed356d4 Compare October 6, 2022 10:12
@arajkumar arajkumar self-assigned this Oct 6, 2022
@arajkumar arajkumar marked this pull request as ready for review October 6, 2022 10:13
@arajkumar arajkumar requested a review from alejandrodnm October 6, 2022 10:15
@arajkumar arajkumar force-pushed the jaeger-storage-int-tests branch from ed356d4 to f04760f Compare October 6, 2022 10:21
@arajkumar arajkumar force-pushed the jaeger-storage-int-tests branch 2 times, most recently from e9a0c7f to d43b582 Compare October 6, 2022 11:12
@arajkumar arajkumar requested a review from cevian October 11, 2022 04:23
@arajkumar
Copy link
Member Author

Similar change in jaeger-clickhouse has been merged.
jaegertracing/jaeger-clickhouse#119

Copy link
Contributor

@cevian cevian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. One minor optional comment. So much better than the copy-the-code approach :)

"event",
"link",
} {
// ignore any error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we ignoring these errors, seem future tests will get messed up if there is an error

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fixed it. Thanks.

@arajkumar arajkumar force-pushed the jaeger-storage-int-tests branch from d43b582 to a241935 Compare October 14, 2022 17:56
@arajkumar arajkumar force-pushed the jaeger-storage-int-tests branch from a241935 to 2a285c5 Compare October 14, 2022 18:03
This commit reuses fixtures from upstream jaeger[1] which is exposed
as golang pkg.

Though we use non release version v1.38.2-0.20221006002917-5bf8a28fe06d, it doesn't have any other changes except [1] which is safe.

[1] jaegertracing/jaeger#3944

Signed-off-by: Arunprasad Rajkumar <[email protected]>
This would help us to live with few failing tests.

Signed-off-by: Arunprasad Rajkumar <[email protected]>
The above test has events which has timestamp difference of ~17hrs when comparing to span.

Signed-off-by: Arunprasad Rajkumar <[email protected]>
Jaeger integration test requires storage specific clean up function which should clear state of previous test execution. Currently we implement the requirement by truncating _ps_trace.{span, event, link} tables which has span specific data.

Signed-off-by: Arunprasad Rajkumar <[email protected]>
Jaeger storage integration tests passes span to SpanWriter and uses same span object to verify against result. We mutate given span to encode binary tags and this causes test change in test expectation.

This commit adds a simple `copyingSpanWriter` which copies tags before passing down to SpanWriter.

Signed-off-by: Arunprasad Rajkumar <[email protected]>
@arajkumar arajkumar force-pushed the jaeger-storage-int-tests branch from 2a285c5 to 6d158d9 Compare October 14, 2022 18:39
@arajkumar arajkumar merged commit 045d01d into timescale:master Oct 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants