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

Align Sampling API with specs #906

Closed
lzchen opened this issue Jul 13, 2020 · 12 comments
Closed

Align Sampling API with specs #906

lzchen opened this issue Jul 13, 2020 · 12 comments
Assignees
Labels
release:required-for-ga To be resolved before GA release sdk Affects the SDK package.

Comments

@lzchen
Copy link
Contributor

lzchen commented Jul 13, 2020

Looks like Sampling "API' is actually defined as part of the SDK.

As well, there are some implementation details that are possibly deviating from the spec.
For example, spans set with is_recording_events to False are currently not even started/ended. According to the spec, it seems the spans should still be passed onto the processor but not the exporter (if is_sampled flag is set).

Also, the is_recording_events field seems to be always returning True as part of the SDK. We probably need this to be an actual property of the span.

@lzchen lzchen added the sdk Affects the SDK package. label Jul 13, 2020
@codeboten codeboten added the release:required-for-ga To be resolved before GA release label Jul 16, 2020
@aravinsiva
Copy link
Contributor

I can look into this if help is still needed.

@aabmass
Copy link
Member

aabmass commented Jul 27, 2020

@lzchen, can Aravin take this or did you already start on it?

@lzchen
Copy link
Contributor Author

lzchen commented Jul 27, 2020

@aravinsiva
Go for it! Thanks.

@aravinsiva
Copy link
Contributor

aravinsiva commented Jul 27, 2020

To summarize what needs to be done:

  • The Sampling API needs to be moved to the SDK
  • And ensuring all corresponding methods meet the spec

Correct? Just want to be sure before I get working

@lzchen
Copy link
Contributor Author

lzchen commented Jul 27, 2020

@aravinsiva
Yes seems about right.

@codeboten
Copy link
Contributor

@aravinsiva are you still interested in taking this on? Just checking that you'll have enough time to work on it.

@aravinsiva
Copy link
Contributor

Hi @codeboten I have done some work on it. However I have a time sensitive project being worked on right now. So if this is high priority perhaps reassigning it is best.

@lzchen
Copy link
Contributor Author

lzchen commented Aug 25, 2020

@lzchen
Copy link
Contributor Author

lzchen commented Aug 25, 2020

@lzchen
Copy link
Contributor Author

lzchen commented Aug 26, 2020

@cijothomas
Copy link
Member

To summarize what needs to be done:

  • The Sampling API needs to be moved to the SDK
  • And ensuring all corresponding methods meet the spec

Correct? Just want to be sure before I get working

the IsRecording of Span must be in the API.
the Sampled flag is part of SpanContext which is part of API.

The actual Samplers, ability to set samplers - part of SDK.

@lzchen lzchen changed the title Align Sampling API with specs + is_recording_events Align Sampling API with specs Sep 2, 2020
@lzchen
Copy link
Contributor Author

lzchen commented Sep 13, 2020

Closed and opening individual issues in the future.

@lzchen lzchen closed this as completed Sep 13, 2020
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this issue Nov 1, 2020
…en-telemetry#906)

* chore: fixing documentation for web tracer provider, fixing examples for web, enable manager when registering

* chore: fixing span extraction from zone after context updates

* chore: bump
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:required-for-ga To be resolved before GA release sdk Affects the SDK package.
Projects
None yet
Development

No branches or pull requests

6 participants