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.

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

Update the custom processor project #996

Merged
merged 8 commits into from
Aug 5, 2020
Merged

Update the custom processor project #996

merged 8 commits into from
Aug 5, 2020

Conversation

reyang
Copy link
Member

@reyang reyang commented Aug 4, 2020

Trying to use the docs and tutorial projects to drive for better design and implementation.
This change should give us some idea about the processor use case.
I plan to work on the sampler and exporter tutorial projects in the next few PRs, then we can use these projects to question ourselves "are these the developer experience we want and are there places we can improve" - similar like what we did here.

Changes

Added multiple processors demo.

@reyang reyang requested a review from a team August 4, 2020 05:51
@reyang reyang added the documentation Documentation related label Aug 4, 2020

// TODO: seems buggy if you remove A and B here, MyActivityProcessor(C).OnEnd is not called.
// TODO: should the dispose order be C, B, A or A, B C?
.AddProcessorPipeline(p => p.AddProcessor(current => new MyActivityProcessor("A")))
Copy link
Member

Choose a reason for hiding this comment

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

This is adding multiple processor pipelines. I guess you wanted to add multiple processors to the same, single pipeline?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, I was using this to showcase how pipeline could be confusing, and I think we need to change it.

Copy link
Member Author

Choose a reason for hiding this comment

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

The fan out scenario is a separate topic, and the name should be changed #979.

public MyActivityProcessor()
private readonly string name;

public MyActivityProcessor(string name)
Copy link
Member

Choose a reason for hiding this comment

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

Need to take the next ActivityProcessor, and call its corresponding methods in Start/End/etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

For demonstration I was trying to make it simple by not chaining the processors at all.

@codecov
Copy link

codecov bot commented Aug 4, 2020

Codecov Report

Merging #996 into master will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #996      +/-   ##
==========================================
+ Coverage   68.41%   68.47%   +0.06%     
==========================================
  Files         220      220              
  Lines        6002     6002              
  Branches      983      983              
==========================================
+ Hits         4106     4110       +4     
+ Misses       1621     1619       -2     
+ Partials      275      273       -2     
Impacted Files Coverage Δ
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 86.76% <0.00%> (+5.88%) ⬆️

@reyang reyang changed the title Update the processor project Update the custom processor project Aug 5, 2020
@cijothomas cijothomas merged commit fd0cc21 into master Aug 5, 2020
@cijothomas cijothomas deleted the reyang/processor branch August 5, 2020 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants