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

Add propagators API #51

Closed
c24t opened this issue Jul 16, 2019 · 6 comments
Closed

Add propagators API #51

c24t opened this issue Jul 16, 2019 · 6 comments
Assignees
Labels
api Affects the API package.
Milestone

Comments

@c24t
Copy link
Member

c24t commented Jul 16, 2019

Add propagators API and propagation methods to the tracing API following the spec.

@Oberon00
Copy link
Member

@toumorokoshi Are you still working on this? Otherwise I'd like to add the ability to get a propagator to the tracer (mirroring the Java implementation), so that integrations can use it.

@Oberon00 Oberon00 added this to the Tracing API milestone Aug 30, 2019
@toumorokoshi
Copy link
Member

@Oberon00 if you want to take a shot, go for it. I would like to tackle this but I have some thoughts about the lack of clarification on the implementation in the spec that I'd like to tackle at the same time. That may take a while to hash out.

So I think an implementation that works as originally spec'd is a good thing to have.

@toumorokoshi
Copy link
Member

Actually I think I have a chance to tackle it this week, probably get a PR out by Tuesday or Wednesday. If you need to move faster go for it.

@Oberon00
Copy link
Member

Oberon00 commented Sep 2, 2019

I don't have a particular need for that particular feature, I just thought this issue might be a leftover from the merged PRs. But if you are still working on it, I will find a different issue. 👌

@toumorokoshi
Copy link
Member

So I've given this area some thought, and I think that this object would be more intuitive if we unify the formatters for distributedcontext and spancontext.

I've outlined an OTEP outlining some other advantages: open-telemetry/oteps#37, but specific to this issue:

I was taking @a-feld's suggestion and creating an object that will consume a context and be responsible for extract and injecting, something like:

class Propagator:

    def inject():
        self.formatter.inject(...)

    def extract() -> 
          self.formatter.extract(...)

To do this, I need the following information:

  • the context to use
  • the httptextformat to use for distributed context
  • the binaryformat to use for distributed context
  • the httptextformat to use for spancontext
  • the binaryformat to use for spancontext
  • the filter of values for distributedcontext (used when injecting)

That would require the constructor to take 6 fields. If I just had a single formatter for both distributedcontext and spancontext, I would only need 4.

thoughts?

@toumorokoshi
Copy link
Member

This is delivered and ready for alpha release, with the merging of #137

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

No branches or pull requests

3 participants