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

Refactor context #564

Open
reyang opened this issue Mar 19, 2019 · 14 comments
Open

Refactor context #564

reyang opened this issue Mar 19, 2019 · 14 comments

Comments

@reyang
Copy link
Contributor

reyang commented Mar 19, 2019

This is to support census-instrumentation/opencensus-specs#247.

  1. Provide a generic context, which is maintained collaboratively by the Python runtime and the OpenCensus library.
  2. Provide asyncio/async/await support.
  3. Refactor existing context (span_context, trace/execution_context, tags/execution_context, correlation_context).
  4. Provide callback API for span start/end.

@c24t @bogdandrutu @discostu105

@bogdandrutu
Copy link

I think this should be under context/ not under contrib. The core library should not depend on a contrib package.

@reyang
Copy link
Contributor Author

reyang commented Mar 20, 2019

@bogdandrutu

  1. Do you mean we want to have this package under context/ instead contrib/ to avoid the confusion?
  2. Or, do you mean this context should be part of the opencensus package instead of having its own package?

Having a small package of the generic context encourages other libraries to interop (similar like gRPC Context in Java) without having to depend on OpenCensus.

@c24t
Copy link
Member

c24t commented Mar 20, 2019

I just talked to @bogdandrutu, he means putting the package under opencensus/context/ or a new top level context/.

@reyang
Copy link
Contributor Author

reyang commented Mar 20, 2019

Thanks for clarification @c24t ! Please let me know your preference on the naming and folder structure.

Considering we will need to move CorrelationContext and TraceContext as well, probably we just rename contrib to a general name packages?

@reyang
Copy link
Contributor Author

reyang commented Mar 20, 2019

Personally I like the way how OpenCensus Node.js is organized, so we avoid inventing 1st class and 2nd class citizens. It also gives us flexibility on having opencensus-api and opencensus-impl in the future.

@c24t
Copy link
Member

c24t commented Mar 20, 2019

Moving everything into a top-level packages/ sounds fine to me, and would make it easy for people browsing the library to find a particular package quickly. But I'm more than happy having first-class opencensus/ and context/ dirs and a second-class contrib/. Up to you.

@bogdandrutu
Copy link

I preferred the way @c24t proposed.

@reyang why do we need to move the CorrelationContext there? Correlation context is part of opencensus/tags. I am not sure I understand.

@reyang
Copy link
Contributor Author

reyang commented Mar 20, 2019

@bogdandrutu CorrelationContext is a separate package which implements the W3C spec.

The opencensus/tags will be implemented leveraging CorrelationContext, while CorrelationContext itself could be used for other purpose (e.g. application developer can put TransactionId and UserIdentity).

@reyang
Copy link
Contributor Author

reyang commented Mar 20, 2019

@bogdandrutu @c24t I don't have strong opinion on the naming and folder structure, so let's use the way @c24t proposed.

@bogdandrutu
Copy link

@reyang I am very confused, my understanding is completely different. The current opencensus/tags are used exactly for this purpose: can be used to construct stats (metrics) or appended to logs or to Spans.

@reyang
Copy link
Contributor Author

reyang commented Mar 20, 2019

@bogdandrutu If the user wants to propagate a TransactionId within/across the process, which has nothing to do with trace/logs/metrics, do we expect CorrelationContext to be used, or we expect opencensus/tags to be used?

@bogdandrutu
Copy link

If TransactionId is used to correlate different telemetry events than that should be in what we currently call tags, if that is for a different purpose I expect them to use a different header.

@reyang
Copy link
Contributor Author

reyang commented Mar 20, 2019

It is for a different purpose (not to correlate telemetry, but to tell the transaction system about the distributed transaction scope).
Do we expect user to use W3C correlation context, or another header should be used (which we don't cover)?
According to the spec, such context could be user's identity, flight name. The spec doesn't require things in the context to be consumed by the telemetry system.

@SergeyKanzhelev
Copy link
Member

@bogdandrutu @reyang we discussed the correlation context and tags relationships with @reyang offline. So the idea that @reyang has is to provide a library that will have inject/extract utility methods for correlation context that can be used outside of Open Census. Like a reference implementation for W3C that somebody can use outside of Open Census.

In Open Census - every name/value pair from CC header will be represented as a tag. And tags API will be the only API to manipulate CC values. Exporter will decide which tags needs to be consumed. Perhaps we may have metadata properties that may categorize tags into telemetry/non-telemetry tags. But it is optional.

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

No branches or pull requests

4 participants