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

Register tracers in dispatcher via method instead of constructor #210

Open
cpx86 opened this issue Jun 15, 2018 · 5 comments
Open

Register tracers in dispatcher via method instead of constructor #210

cpx86 opened this issue Jun 15, 2018 · 5 comments

Comments

@cpx86
Copy link
Contributor

cpx86 commented Jun 15, 2018

Just realized I missed a very important detail with #208.

The way the TraceManager handles the default dispatcher, it registers it's internal Push method via the constructor of the dispatcher (see https://github.com/openzipkin/zipkin4net/blob/master/Src/zipkin4net/Src/TraceManager.cs#L61). This is obviously not possible for any dispatcher defined outside of zipkin4net.

One way to solve this would be to extend the IRecordDispatcher interface so that the tracer action is registered via a method instead of the constructor. Either

  1. Add the Action<Record> parameter to the existing Record method
  2. Add a new method e.g. RegisterTracers

Thoughts?

@fedj
Copy link
Collaborator

fedj commented Jun 15, 2018

Hi @cpx86,

I'm not sure to understand. Can't you use Start(ILogger, IRecordDispatcher) ?

@fedj
Copy link
Collaborator

fedj commented Jun 15, 2018

Oh, just saw the problem, let me check

@fedj
Copy link
Collaborator

fedj commented Jun 15, 2018

It weirdly sound to me that tracers should be registered in dispatchers. I would vote for a RegisterTracer method in Dispatcher.

@cpx86
Copy link
Contributor Author

cpx86 commented Jun 15, 2018

I think that makes sense as well.

Another question - should the dispatcher receive an pre-baked Action<Record> or would it make sense to just send an IEnumerable<Tracer> instead? I'm leaning towards the latter - feels more clean.

@fedj
Copy link
Collaborator

fedj commented Jun 15, 2018

Indeed, feels cleaner as it would separate concerns. Can you come up with a PR ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants