Opentracing minimal changes for processors #37837
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robot-ch-test-poll
added
the
pr-not-for-changelog
|
|
@Mergifyio update |
✅ Branch has been successfully updated |
|
@Mergifyio update |
✅ Branch has been successfully updated |
|
@tavplubix can you tell that this changes are (not) related to this issue #37920? |
|
Most likely it's not related |
nikitamikhaylov
approved these changes
Jun 9, 2022
tavplubix
reviewed
Jun 15, 2022
| @@ -70,6 +71,7 @@ static void executeJob(ExecutingGraph::Node * node, ReadProgressCallback * read_ | |||
|
|
|||
| bool ExecutionThreadContext::executeTask() | |||
| { | |||
| OpenTelemetrySpanHolder span("ExecutionThreadContext::executeTask() " + node->processor->getName()); | |||
There was a problem hiding this comment.
Seems like it creates too many spans, so 00974_query_profiler sometimes fail with
<Error> SystemLog (system.opentelemetry_span_log): Queue is full for system log 'DB::OpenTelemetrySpanLog' at 955525
Example: https://s3.amazonaws.com/clickhouse-test-reports/0/e13dc23d344454ecda401df4b23b696a691421fe/stateless_tests__release__databaseordinary__actions_.html
More failures: https://play.clickhouse.com/play?user=play#c2VsZWN0IAp0b1N0YXJ0T2ZEYXkoY2hlY2tfc3RhcnRfdGltZSkgYXMgZCwKY291bnQoKSwgZ3JvdXBVbmlxQXJyYXkocHVsbF9yZXF1ZXN0X251bWJlciksICBhbnkocmVwb3J0X3VybCkKZnJvbSBjaGVja3Mgd2hlcmUgJzIwMjItMDQtMDEnIDw9IGNoZWNrX3N0YXJ0X3RpbWUgYW5kIHRlc3RfbmFtZSBsaWtlICclMDA5NzRfcXVlcnlfcHJvZmlsZXIlJyBhbmQgdGVzdF9zdGF0dXMgaW4gKCdGQUlMJywgJ0ZMQUtZJykgZ3JvdXAgYnkgZCBvcmRlciBieSBkIGRlc2M=
@qoega, should we revert it?
There was a problem hiding this comment.
not all of them. Probably some.
Too many spans is not the reason we have sanitiser alerts. For 00974_query_profiler we better just disable opentelemetry_span_log with 'set opentelemetry_start_trace_probability=1'. It is enabled there by random settings fuzzer.
I will build asan debug build and will try to reproduce sanitiser alert.
There was a problem hiding this comment.
Too many spans is not the reason we have sanitiser alerts.
Yes, most likely it's two independent issues.
For 00974_query_profiler we better just disable opentelemetry_span_log with 'set opentelemetry_start_trace_probability=1'.
Are you sure that it will not produce to many spans in a production environment?
This was referenced Jun 16, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Instead of #37186