-
Notifications
You must be signed in to change notification settings - Fork 896
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SDK tracing: clarify multi-processors scenarios #338
Merged
Merged
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
b357525
Recommend chaining on processors
9724a8c
fix lint
dac6f49
fix lint2
9cf2f48
example for span chaining
0634a69
fix formatting
3cea141
Update sdk-tracing.md
4e469e7
Update sdk-tracing.md
ab69437
Update sdk-tracing.md
0a9ef03
remove advanced example, do not encourage implementing helpers in the…
238945f
recommend to move implement new common processing scearios out-of-proc
f92579f
fix lint
7761327
BatchProcessor -> BatchEporterProcessor
6e6bda3
Merge branch 'master' into patch-3
SergeyKanzhelev File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lmolkova I understand that you are clarifying the document and not suggesting new functionality but if you don't mind I would like to bring attention of approvers to this issue while we are working on this part of the spec.
@open-telemetry/specs-approvers what are the arguments for having multi-pipeline feature in-process rather than use the already existing equivalent functionality in OpenTelemetry Collector? In my opinion this could be removed from the SDK and we encourage using Collector for this, but perhaps there are use cases that I am not aware of that require this to be in SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it can be removed from SDK. There are definitely cases when collector cannot be installed. Also some spans can be filtered out right from the process to save on cross-proc communication with collector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tigrannajaryan for the z-pages functionality that we want to support it is inefficient to stream everything to the collector. So at least one example that we want to support is:
sampled
spans to the configure exporter (collector probably)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to Sergey and Bogdan's comments
I don't think that having equivalent functionality is bad in this case (and I say this as someone who expects most of their customers to use the collector). As Sergey mentioned, there will always be cases where people can't use the collector or where it doesn't add much value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍