Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Need to rename BroadcastActivityProcessor #979

Closed
reyang opened this issue Aug 3, 2020 · 8 comments
Closed

Need to rename BroadcastActivityProcessor #979

reyang opened this issue Aug 3, 2020 · 8 comments
Labels
bug Something isn't working help wanted Good for taking. Extra help will be provided by maintainers pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package

Comments

@reyang
Copy link
Member

reyang commented Aug 3, 2020

#735 (comment)
@reyang reyang added bug Something isn't working pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package priority:p1 help wanted Good for taking. Extra help will be provided by maintainers labels Aug 3, 2020
@eddynaka
Copy link
Contributor

eddynaka commented Aug 3, 2020

Are we going to change to FanOutActivityProcessor ?

@reyang reyang mentioned this issue Aug 4, 2020
Merged
@reyang
Copy link
Member Author

reyang commented Aug 4, 2020

I'm fine with either AggregatedActivityProcessor, FanOutActivityProcessor or MultiActivityProcessor, as long as it is not misleading / having broadcast.
Let's see what @alanwest @cijothomas @CodeBlanch would recommend.

@CodeBlanch
Copy link
Member

For some reason, I feel strongly against AggregatedActivityProcessor. Seems to suggest we are aggregating activity/spans somehow?

Either FanOutActivityProcessor or MultiActivityProcessor I'm good with. I think technically "fan out" reflects what is going on the best. But MultiActivityProcessor is used by other repos (more or less) and there is some value in sharing names/concepts.

If I had 3 points to assign: 2 FanOutActivityProcessor, 1 MultiActivityProcessor, 0 AggregatedActivityProcessor.

@alanwest
Copy link
Member

alanwest commented Aug 4, 2020

For some reason, I feel strongly against AggregatedActivityProcessor. Seems to suggest we are aggregating activity/spans somehow?

Agreed that Aggregated does not inspire the correct meaning in my mind.

But MultiActivityProcessor is used by other repos (more or less) and there is some value in sharing names/concepts.

I'd lean more towards this argument that Mutli is a good way to go. I do see the appeal of FanOut, though the only context I've seen this in is messaging systems (that said, there may be other contexts I'm not thinking of, so I could be swayed)

@cijothomas
Copy link
Member

The functionality done by the BroadCastActivityProcessor is not covered in spec. Spec currently has Simple/batching, and does not mention anything about "fan out or broadcast". Spec also talks about multiple pipelines - but I am guessing we can achieve multiple pipelines by instantiating multiple TracerProviders, each with own config. If we do that, then we would no longer need the equivalent of BroadCastProcessor.

@reyang
Copy link
Member Author

reyang commented Aug 5, 2020

#923 is using the word Composite 😄

@eddynaka
Copy link
Contributor

eddynaka commented Aug 5, 2020

#923 is using the word Composite 😄

Took from the spec.

@eddynaka eddynaka mentioned this issue Aug 5, 2020
Merged
2 tasks
@eddynaka
Copy link
Contributor

@reyang @cijothomas , we can close this.

@reyang reyang closed this as completed Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working help wanted Good for taking. Extra help will be provided by maintainers pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@alanwest @cijothomas @eddynaka @reyang @CodeBlanch